Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite for named tuples #852

Open
wants to merge 93 commits into
base: main
Choose a base branch
from

Conversation

jsfreischuetz
Copy link
Contributor

@jsfreischuetz jsfreischuetz commented Aug 21, 2024

Title

Refactor mlos_core APIs to encapsulate related data fields.

Description

Refactors the mlos_core Optimizer APIs to accept new data types Observation, Observations and return Suggestion, instead of a mess of Tuple[DataFrame, DataFrame, Optional[DataFrame], Optional[DataFrame]] that must be named and checked everywhere.

Additionally, this makes it more explicit that _register is a bulk operation that is not actually supported currently by the underlying optimizers, though leaves notes on how we can do that in the future.

Type of Change

  • Refactor

Testing

Usual CI plus some new unit tests for new data type operations.


Additional Notes

A more significant rewrite of named tuple support inside mlos_core.
This is based on comments in #811 as well as conversations with @bpkroth


@bpkroth
Copy link
Contributor

bpkroth commented Nov 22, 2024

All good now. Thanks for your patience :)

@bpkroth bpkroth enabled auto-merge (squash) November 22, 2024 02:29
@bpkroth bpkroth enabled auto-merge (squash) November 22, 2024 02:34
@bpkroth bpkroth added the WIP Work in progress - do not merge yet label Nov 22, 2024
@bpkroth bpkroth self-requested a review November 22, 2024 18:03
@bpkroth
Copy link
Contributor

bpkroth commented Nov 22, 2024

Found a few more issues. Gonna try and tackle them myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress - do not merge yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants