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

Cleaned emref module and adapted other to new env variables #140

Merged
merged 3 commits into from
Nov 24, 2021

Conversation

amjjbonvin
Copy link
Member

Docking example now works including the emref module.
Adapted all previous modules to use the new environment variables (related to issue
Next step is mdref

PS: The environment variables are printed in the output for each model - must be an old debug statement remaining somewhere in the python code

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #140 (0b76784) into main (2dea812) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   39.61%   39.61%           
=======================================
  Files          39       39           
  Lines        2148     2148           
=======================================
  Hits          851      851           
  Misses       1297     1297           
Impacted Files Coverage Δ
src/haddock/libs/libsubprocess.py 34.00% <ø> (+0.66%) ⬆️
src/haddock/modules/refinement/emref/__init__.py 22.66% <0.00%> (-0.31%) ⬇️
src/haddock/modules/refinement/flexref/__init__.py 22.66% <0.00%> (ø)
src/haddock/modules/refinement/mdref/__init__.py 22.97% <0.00%> (ø)
src/haddock/modules/sampling/gdock/__init__.py 20.45% <0.00%> (ø)
src/haddock/modules/sampling/rigidbody/__init__.py 22.07% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dea812...0b76784. Read the comment docs.

Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

194 files, impossible to check everything, will trust @amjjbonvin also on this one!

@@ -108,7 +110,7 @@ def run(self, **params):
self.path,
self.recipe_str,
self.params,
ambig_f=self.params.get('ambig', None),
ambig=self.params.get('ambig', None),
Copy link
Member

Choose a reason for hiding this comment

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

This was a good bug!

@joaomcteixeira
Copy link
Member

PS: The environment variables are printed in the output for each model - must be an old debug statement remaining somewhere in the python code

taking note of this

@joaomcteixeira
Copy link
Member

joaomcteixeira commented Nov 23, 2021

I agree with Rodrigo. Got ahead and merge this PR on your own 🚀 🎸 🔥

@joaomcteixeira joaomcteixeira added the enhancement Enhancing an existing feature of adding a new one label Nov 23, 2021
@joaomcteixeira joaomcteixeira added this to In progress in CNS via automation Nov 23, 2021
@joaomcteixeira joaomcteixeira added this to the v3.0.0 stable release milestone Nov 23, 2021
Copy link
Member

@joaomcteixeira joaomcteixeira left a comment

Choose a reason for hiding this comment

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

Give me some time to clear some small bugs relating to var names.

@@ -108,7 +110,7 @@ def run(self, **params):
self.path,
self.recipe_str,
self.params,
ambig_f=self.params.get('ambig', None),
ambig=self.params.get('ambig', None),
Copy link
Member

Choose a reason for hiding this comment

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

There is a bug here with ambig not being ambig_fname. I will correct that and inspect also other things. Please wait before merging.

@joaomcteixeira joaomcteixeira marked this pull request as draft November 24, 2021 08:27
@joaomcteixeira
Copy link
Member

The ambig_fname was not being loaded due to an error in the variable name being pulled from the param dictionary ambig instead of ambig_fname. I am correcting that and working on a way to block future var typing errors at this stage. Var typing errors are already corrected when writing the config.

@joaomcteixeira joaomcteixeira marked this pull request as ready for review November 24, 2021 09:18
@joaomcteixeira
Copy link
Member

@amjjbonvin I corrected the capturing of ambig_fname parameter. Before there was an error and the ambig file was NOT being loaded into the CNS .inp file because self.params.get('ambig', None) was being used and masked the bug. Now that ambig_fname = "" is part of the defaults.cfg this can be rewritten to a normal self.params['ambig_fname'].

You may wish to review the calculations again after this change.

@amjjbonvin
Copy link
Member Author

amjjbonvin commented Nov 24, 2021 via email

@joaomcteixeira
Copy link
Member

Yes, I checked the other modules. All corrected now. The new changes are already part of the of this branch. When the PR it merged the new changes will go along. I was working on #138 from this branch. If you want we can solve that before merging this one. Would be best I think.

@amjjbonvin
Copy link
Member Author

amjjbonvin commented Nov 24, 2021 via email

@joaomcteixeira
Copy link
Member

Perfect. Converted to draft until #142 is solved.

@joaomcteixeira joaomcteixeira marked this pull request as ready for review November 24, 2021 16:28
@joaomcteixeira
Copy link
Member

Because we closed #142, I think this is safe to merge. Whatever comes after #142 is big enough to hold by itself.

@amjjbonvin amjjbonvin merged commit 6e1002b into main Nov 24, 2021
CNS automation moved this from In progress to Done Nov 24, 2021
@amjjbonvin amjjbonvin deleted the haddock3-emref branch November 24, 2021 16:30
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
Projects
CNS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants