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

Implement ConfigIDs and rename objective function budget to fidelity #66

Merged
merged 9 commits into from
Nov 10, 2023

Conversation

Bronzila
Copy link
Collaborator

@Bronzila Bronzila commented Oct 27, 2023

Implement ConfigID functionality by utilizing a new class ConfigRepository.
While there is still the possibility to extend the functionality e.g. for saving states, logs etc., I wanted to open this PR to get some continous feedback on my design.

Moreover I have renamed all budgets referring to the objective function to fidelity as we have discussed.

Please feel free to leave your comments/ideas on the changes!

@Bronzila Bronzila requested a review from Neeratyoy October 27, 2023 15:06
Comment on lines 558 to 561
if not hasattr(self, 'traj') or reset:
self.reset()
if verbose:
print("Initializing and evaluating new population...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This caught my eye.

This traj could be confusing as its role of reset appears important.

Given we have moved to ResultItem and nice data classes, can we also store these traj like data variables better?
If you have a solution in mind, please feel free to raise an issue with it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code block seems rather unituitive for me anyways. We are restriciting the user to always start a new run when calling the run method. I would personally prefer leaving this check out generally and leaving the users to reset the optimizer when ever they want. This would then be similar to the behavior in DEHB.

For the traj like classes I do agree to store them in a more principled fashion. I will think about it!

Copy link
Collaborator

@Neeratyoy Neeratyoy left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks!

Could we add a simple check (unit test) that ensures that the generated config IDs for a single worker DEHB run are as expected?

@Bronzila Bronzila merged commit a7f6bd9 into development Nov 10, 2023
@Bronzila Bronzila deleted the feat/id_list branch November 14, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants