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

Issuefixes #658

Merged
merged 9 commits into from
Aug 24, 2023
Merged

Issuefixes #658

merged 9 commits into from
Aug 24, 2023

Conversation

VGPReys
Copy link
Contributor

@VGPReys VGPReys commented Jun 5, 2023

You are about to submit a new Pull Request. Before continuing make sure you read the contributing guidelines and that you comply with the following criteria:

  • You have sticked to Python. Please talk to us before adding other programming languages to HADDOCK3
  • Your PR is about CNS
  • Your code is well documented: proper docstrings and explanatory comments for those tricky parts
  • You structured the code into small functions as much as possible. You can use classes if there is a (state) purpose
  • Your code follows our coding style
  • You wrote tests for the new code
  • tox tests pass. Run tox command inside the repository folder
  • -test.cfg examples execute without errors. Inside examples/ run python run_tests.py -b
  • PR does not add any dependencies, unless permission granted by the HADDOCK team
  • PR does not break licensing
  • Your PR is about writing documentation for already existing code 🔥
  • Your PR is about writing tests for already existing code :godmode:

This PR aims at closing three issues:

  • With the addition of a regular expression that checks the presence of upper case True and False in the .toml input file and lowering them on-the-fly, this closes Booleans with capitals #575
  • Also by checking values types and ranges/choices, by comparing values present in the defaults.yaml and parameter = value present in the user provided .cfg file, this closes check values in cfg files against .yaml files  #487
  • Finally, a bunch of copies of the workflow haddock3 input files are now written in the data directory of the run. This includes the original inputed file, the regular expression cleaned file and the enhanced full parameter file, which includes all default parameters in addition to the ones provided by the user. This closes Adding the config file to the data dir of a run #578

Accordingly, additionnal tests were added, while some other test were modified to fit the new implementation.

Finally, run_tests.py ran without errors and the comparisons by compare-runs.py returned only No errors found - OKAY!.

…er case hence on-the-fly debugging possible user-provided toml configuration files.
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2023

Codecov Report

Patch coverage: 64.17% and project coverage change: -0.22 ⚠️

Comparison is base (69ee732) 74.51% compared to head (934d32b) 74.29%.

❗ Current head 934d32b differs from pull request most recent head aec8259. Consider uploading reports for the commit aec8259 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
- Coverage   74.51%   74.29%   -0.22%     
==========================================
  Files         111      111              
  Lines        7572     7691     +119     
==========================================
+ Hits         5642     5714      +72     
- Misses       1930     1977      +47     
Impacted Files Coverage Δ
src/haddock/gear/prepare_run.py 48.52% <45.67%> (-1.33%) ⬇️
src/haddock/gear/config.py 79.59% <62.50%> (-1.93%) ⬇️
src/haddock/gear/yaml2cfg.py 93.84% <83.33%> (-1.40%) ⬇️
tests/test_gear_config.py 100.00% <100.00%> (ø)
tests/test_gear_prepare_run.py 99.33% <100.00%> (+0.10%) ⬆️
tests/test_modules_general.py 97.27% <100.00%> (+0.12%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@VGPReys VGPReys added enhancement Enhancing an existing feature of adding a new one yaml default parameters Anything related to the YAML configuration files for default parameters labels Jun 7, 2023
Copy link
Contributor

@mgiulini mgiulini left a comment

Choose a reason for hiding this comment

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

added a few comments, I will test it asap!

src/haddock/gear/prepare_run.py Show resolved Hide resolved
src/haddock/gear/prepare_run.py Show resolved Hide resolved
if confname not in added_files.keys():
continue
f.write(f'"{added_files[confname]}": {infofile[confname]}\n')

Copy link
Contributor

Choose a reason for hiding this comment

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

I would return something here, like the paths to the saved file

src/haddock/gear/prepare_run.py Outdated Show resolved Hide resolved
src/haddock/gear/yaml2cfg.py Outdated Show resolved Hide resolved
Comment on lines 133 to 139
Return
------
ycfg : dict
The full default configuration file as a dict
OR
cfg : dict
A dictionnary containing only the default parameters values
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any particular reason to have two different dict objects in output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is the original one cfg that only contains the ['default'] value and used in other parts of the code.
While the ycfg one contrains verything present in the default.yaml file loaded as a dict, used for the analysis of the agreement (type, range, choices, sizes) between user provided configuration and authorized one.

@rvhonorato rvhonorato added workflow All the general parts of HADDOCK3 not related to any module in particular m|emscoring Related to emscoring module m|emref emref module m|flexref flexref module m|mdref mdref module m|mdscoring mdscoring module labels Jun 7, 2023
@rvhonorato
Copy link
Member

Sorry I have absolutely no idea about this part of the code, will trust your judgment 🤞🏽

@rvhonorato rvhonorato removed their request for review June 7, 2023 13:57
VGPReys and others added 3 commits June 7, 2023 16:05
I still have orthographic issues...

Co-authored-by: Marco Giulini <54807167+mgiulini@users.noreply.github.com>
@VGPReys VGPReys requested a review from mgiulini August 11, 2023 08:59
Copy link
Contributor

@mgiulini mgiulini left a comment

Choose a reason for hiding this comment

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

Runs nicely on a couple of examples, great addition @VGPReys!

@VGPReys VGPReys merged commit e6cca18 into main Aug 24, 2023
8 checks passed
@VGPReys VGPReys deleted the issuefixes branch August 24, 2023 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancing an existing feature of adding a new one m|emref emref module m|emscoring Related to emscoring module m|flexref flexref module m|mdref mdref module m|mdscoring mdscoring module workflow All the general parts of HADDOCK3 not related to any module in particular yaml default parameters Anything related to the YAML configuration files for default parameters
Projects
None yet
4 participants