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.

Sign up for GitHub

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

Jump to bottom

Objective classes #188

Merged
merged 46 commits into from
Apr 10, 2024
Merged

Objective classes #188

merged 46 commits into from
Apr 10, 2024

Conversation

AdrianSosic
Copy link
Collaborator

This PR introduces the new objective classes SingleTargetObjective and DesirabilityObjective as replacements for the former Objective class, together with an appropriate deprecation mechanism.

@AdrianSosic AdrianSosic added the enhancement Expand / change existing functionality label Apr 2, 2024
@AdrianSosic AdrianSosic self-assigned this Apr 2, 2024
@AdrianSosic
Copy link
Collaborator Author

Hi @Scienfitz and @AVHopp, the objective PR is finally ready. I know it seems like a lot of changes, but please be aware that in most files only the references were updated, so it's not as much as it appears at the first glance. Also, I've created one initial commit where I copied the plain objective source code to the new subclass files, so you can also see diffs if you compare against that commit. Other than that, there was really no better way to break this PR down to smaller changes...

But let me know what you think 🙃

AdrianSosic

This comment was marked as outdated.

Sign in to view

AVHopp
AVHopp reviewed Apr 4, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
baybe/objectives/base.py Show resolved Hide resolved
baybe/objectives/base.py Outdated Show resolved Hide resolved
baybe/objective.py Show resolved Hide resolved
baybe/objectives/base.py Outdated Show resolved Hide resolved
baybe/targets/base.py Show resolved Hide resolved
baybe/targets/numerical.py Outdated Show resolved Hide resolved
baybe/utils/basic.py Outdated Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
Scienfitz
Scienfitz reviewed Apr 4, 2024
View reviewed changes
baybe/objectives/enum.py Outdated Show resolved Hide resolved
baybe/objectives/single.py Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
tests/test_deprecations.py Show resolved Hide resolved
baybe/objectives/desirability.py Outdated Show resolved Hide resolved
baybe/objectives/deprecation.py Outdated Show resolved Hide resolved
baybe/recommenders/deprecation.py Show resolved Hide resolved
baybe/objectives/single.py Outdated Show resolved Hide resolved
baybe/objectives/desirability.py Outdated Show resolved Hide resolved
AdrianSosic added 19 commits April 8, 2024 16:11
@AdrianSosic
Copy objective.py to objectives/single.py and objectives/desirability.py
fea428d
@AdrianSosic
Rename new classes
3229481
@AdrianSosic
Remove __str__ methods
cb8cffd
@AdrianSosic
Remove docstrings
51abc24
@AdrianSosic
Remove mode attributes and code parts that are inconsistent with the …
a4bb30e 
…mode
@AdrianSosic
Add Objective base class
516853a
@AdrianSosic
Implement dispatching mechanism for backward compatibility
d0879bd
@AdrianSosic
Enable serialization with automatic fallback type selection
dce3c67
@AdrianSosic
Refactor single.py
01e71b9
@AdrianSosic
Introduce enum for combine_func
146cbd9
@AdrianSosic
Properly handle dependency on NumericalTarget
3a36539
@AdrianSosic
Change desirability weight normalization to 1
e29490e
@AdrianSosic
Improve weight conversion logic
2796f62
@AdrianSosic
Use tuples instead of lists
b1dcac3
@AdrianSosic
Add missing converter and validators
4e51fa9
@AdrianSosic
Rework docstrings and comments
cd9c750
@AdrianSosic
Introduce is_normalized property for targets
5939872 
This solves the Law of Demeter violation in `DesirabilityObjective`, whose validation step previously relied on a non-obvious implementation detail of the target class. A modification of the target code would have been at risk of silently breaking the target code.
@AdrianSosic
Fix typing issues
0148b29
@AdrianSosic
Extract scalarization function and add test
638b264
@AdrianSosic AdrianSosic force-pushed the refactor/objectives branch from a327984 to 54189cf Compare April 8, 2024 14:19
@AdrianSosic
Copy link
Collaborator Author

Hi @AVHopp @Scienfitz. All open issues have been addressed and the PR is now ready for merge from my side (except for that one open issue where I would appreciate some suggestions, @Scienfitz).

BTW: decided to update via merge instead of rebase because this would have been really error-prone...

@AdrianSosic AdrianSosic mentioned this pull request Apr 9, 2024
Merged
AVHopp
AVHopp approved these changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good :)

@AdrianSosic
Refactor is_normalized property
0cc5b58 
* Rename to _is_transform_normalized
* Improve docstring and error message
@AdrianSosic
Add mypy entry to CHANGELOG.md
f306dfa
@AdrianSosic AdrianSosic added this to the Mypy milestone Apr 10, 2024
@AdrianSosic AdrianSosic merged commit d0e99be into main Apr 10, 2024
10 checks passed
@AdrianSosic AdrianSosic deleted the refactor/objectives branch April 10, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AVHopp AVHopp AVHopp approved these changes

@Scienfitz Scienfitz Scienfitz approved these changes

Assignees

@AdrianSosic AdrianSosic

Labels
enhancement Expand / change existing functionality
Projects
None yet
Milestone
Mypy
Development

Successfully merging this pull request may close these issues.
3 participants
@AdrianSosic @Scienfitz @AVHopp