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

Change config handling #410

Open
wants to merge 21 commits into
base: refactor_config
Choose a base branch
from
Open

Change config handling #410

wants to merge 21 commits into from

Conversation

mschwoer
Copy link
Collaborator

@mschwoer mschwoer commented Dec 16, 2024 •
edited
Loading

  • change the handling of the config. The idea is to have a config dumped to disk that is fully self-contained, i.e. no additional CLI parameters are required.
  • strictly forbid to add new keys (as they would not be understood by AlphaDIA anyway), this requires change the modifications section
  • refactor what is passed to SearchPlan

See the updated docs for the motivation :-)

@mschwoer mschwoer requested a review from GeorgWa December 16, 2024 10:31
mschwoer
mschwoer commented Dec 18, 2024
View reviewed changes
alphadia/constants/default.yaml
@@ -55,31 +66,31 @@ library_prediction:
# also used for decoy channels
custom_modifications:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alternatively: allow to add keys for the custom_modifications key

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update: now that we can overwrite lists, the original format can be used again

@mschwoer mschwoer force-pushed the change_config_handling branch from e7f8988 to 6c8854b Compare December 18, 2024 16:39
GeorgWa
GeorgWa reviewed Dec 18, 2024
View reviewed changes
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.

Looks good!

alphadia/cli.py Outdated
cli_params_config = {
ConfigKeys.RAW_PATHS: parse_raw_path_list(args, config),
ConfigKeys.LIBRARY_PATH: args.library,
ConfigKeys.FASTA_PATHS: args.fasta,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The output path is not here because it’s the only strictly required parameter I guess?

alphadia/constants/multistep.yaml Outdated
@@ -12,6 +12,7 @@ transfer:
enabled: True

# override settings that could have been set by the user:
quant_dir: null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, to overwrite anything the user was setting

alphadia/libtransform.py Show resolved Hide resolved
alphadia/search_plan.py
mbr_step_extra_config = (
self._multistep_config[MBR_STEP_NAME] | optimized_values_config
self._multistep_config[MBR_STEP_NAME]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a way to update dicts?🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*merge dicts

alphadia/workflow/base.py
@@ -52,7 +54,7 @@ def __init__(
"""
self._instance_name: str = instance_name
self._parent_path: str = quant_path or os.path.join(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should rename the parent_path. That’s indeed quite confusing.

alphadia/workflow/config.py Outdated
@@ -33,7 +33,9 @@

logger = logging.getLogger()

USER_DEFINED = "user defined"
USER_DEFINED_CLI = "user defined (cli)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also what gui users will see. I guess it’s fine 🤷🏻‍♂️

alphadia/workflow/config.py Outdated
return new_value

# If the default config is a list, then we need to update each item in the list
# If the default config is a list, then we overwrite all items in the list. The latest experiment wins.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this! This is totally sufficient and simpler.

docs/guides/transfer-dimethyl.md
@@ -153,3 +153,7 @@ multistep_search:
transfer_step_enabled: True
mbr_step_enabled: True
```

In case the multistep search fails at some step, you can restart the failed step by
using the `full_config.yaml` that is stored in the respective subfolder. You can of course edit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this applicable to GUI users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not yet

@mschwoer mschwoer force-pushed the change_config_handling branch from ea3c760 to 25c4bc0 Compare December 19, 2024 06:37
@mschwoer mschwoer force-pushed the change_config_handling branch from 70560fa to 82dfe46 Compare December 20, 2024 14:48
@mschwoer mschwoer changed the base branch from rename_plan to refactor_config December 20, 2024 14:49
@mschwoer mschwoer marked this pull request as ready for review December 20, 2024 14:51
@mschwoer mschwoer requested a review from mo-sameh December 20, 2024 15:38
@GeorgWa GeorgWa self-requested a review December 21, 2024 18:45
GeorgWa
GeorgWa approved these changes Dec 21, 2024
View reviewed changes
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgWa GeorgWa GeorgWa approved these changes

@mo-sameh mo-sameh Awaiting requested review from mo-sameh

Assignees
No one assigned
Labels
breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mschwoer @GeorgWa