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

Multistep search #399

Open
wants to merge 42 commits into
base: development
Choose a base branch
from
Open

Multistep search #399

wants to merge 42 commits into from

Conversation

mschwoer
Copy link
Collaborator

@mschwoer mschwoer commented Dec 10, 2024

version 1:

A first very rough outline, just to get some early feedback.

The idea is to activate the MSS via the config parameter
multistep_search: enabled: True
The same config key holds the special parameters required for each step, which overwrite default.yaml.
Orchestration of the workflow (and the folders) is done as code.

See individual commit messages for an outline of the ideas.

update 1:

Refactored according to feedback.
Decided still against having everything in one folder, as it makes life much harder: the prefix would need to be made > known to a lot of parts in the code, and if you miss one you might tacitly overwrite files (but open for discussion). The last > step will always write to "output_dir", such that the "final" results are always at the same place.

I am happy enough with the overall structure, among the open questions for this PR would be the exact settings in >multistep.yaml (see also the added TODOs which show some additional uncertainties).

update 2:
This PR adds the multistep search including tests, UI, docs, and e2e tests.

@mschwoer mschwoer requested a review from GeorgWa December 10, 2024 15:19
@mschwoer mschwoer changed the base branch from development to refactoring December 10, 2024 15:20
@GeorgWa
Copy link
Collaborator

GeorgWa commented Dec 10, 2024

I think this fits the idea very well apart from how the user triggers it.

Ideally the user should have two Boolean arguments:

transfer_step: true
mbr_step: true 

the first one will prepend a transfer learning to the main search, the second one will append the MBR search to the main search.

The config specification of what should be overwritten looks good.

@mschwoer mschwoer added the test:e2e End to end tests will be run on PRs that carry this label. label Dec 11, 2024
@mschwoer mschwoer added test:e2e End to end tests will be run on PRs that carry this label. and removed test:e2e End to end tests will be run on PRs that carry this label. labels Dec 11, 2024
Copy link
Collaborator

@GeorgWa GeorgWa left a comment

Choose a reason for hiding this comment

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

LGTM

alphadia/constants/multistep.yaml Outdated Show resolved Hide resolved
library:
general:
save_library: False
library_prediction: # needs to be done if transfer AND/OR mbr enabled?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional if MBR search, mandatory if TL search

alphadia/constants/multistep.yaml Outdated Show resolved Hide resolved

mbr:
general:
reuse_quant: True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, reuse_quant is not needed.

alphadia/planning.py Outdated Show resolved Hide resolved
@@ -328,13 +347,18 @@ def run(
logger.progress("Starting Search Workflows")

workflow_folder_list = []
single_estimators = defaultdict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I use estimator already in the calibration. This is the results of the hyper parameter optimisation. So it could be optimised parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea, but will be read from stats.tsv anyway

alphadia/search_plan.py Show resolved Hide resolved
@mschwoer mschwoer added test:e2e End to end tests will be run on PRs that carry this label. and removed test:e2e End to end tests will be run on PRs that carry this label. labels Dec 12, 2024
@mschwoer mschwoer added test:e2e End to end tests will be run on PRs that carry this label. and removed test:e2e End to end tests will be run on PRs that carry this label. labels Dec 12, 2024
@mschwoer mschwoer marked this pull request as ready for review December 13, 2024 14:26
@mschwoer mschwoer changed the title Outline for multistep search Multistep search Dec 13, 2024
@mschwoer mschwoer added test:e2e End to end tests will be run on PRs that carry this label. and removed test:e2e End to end tests will be run on PRs that carry this label. labels Dec 13, 2024
Base automatically changed from refactoring to development December 20, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:e2e End to end tests will be run on PRs that carry this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants