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

Refactor config #411

Open
wants to merge 14 commits into
base: add_unit_test_for_config
Choose a base branch
from
Open

Refactor config #411

wants to merge 14 commits into from

Conversation

mschwoer
Copy link
Collaborator

@mschwoer mschwoer commented Dec 19, 2024

Refactor the config class:

  • separate merging and printing (up to the tracking_config variable)
  • add more fine grained unit tests

@mschwoer mschwoer added the test:e2e End to end tests will be run on PRs that carry this label. label Dec 19, 2024
@mschwoer mschwoer marked this pull request as ready for review December 20, 2024 14:21
@mschwoer mschwoer force-pushed the add_unit_test_for_config branch from d91f9be to 42b4798 Compare December 20, 2024 14:44
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.

Much cleaner implementation! LGTM

@@ -26,7 +31,7 @@ library_prediction:
variable_modifications: 'Oxidation@M;Acetyl@Protein_N-term'
max_var_mod_num: 2
missed_cleavages: 1
precursor_len:
precursor_len: # TODO: make this min/max ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way how the gui interacts with the config makes it easier if this is a treated like a fixed length tuple.

do_print : bool, optional
Whether to print the modified config. Default is False.
"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a sentence about the strategy of using a tracking config

"""
Recursively update target_dict in-place with values from update_dict, following specific rules for different types.

For each value that gets updated, the corresponding value in tracking_dict is updated with experiment_name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable name experiment_name was a bit of a misstranslation which we kept for too long. Let's rename it here.


For each value that gets updated, the corresponding value in tracking_dict is updated with experiment_name.

Args:
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) numpy style docstrings

"""
for key, update_value in update_config.items():
if key not in target_config:
raise ValueError(f"Key not found in target_config: '{key}'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this more explicit?
Something like: Updating configs does not allow defining new keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change 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