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

Implement the get_options method for JobCalculation #1961

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 12, 2018

Fixes #1533

What is currently defined as the options for a JobCalculation, are specific
attributes that were also stored as node attributes through set_ methods. These
were called setters and are all defined explicitly, along with the corresponding
getters. With the introduction of the Process paradigm, these could no longer be
called manually by the user on the calculation node, and so the options dictionary
was invented, to allow the user to specify these attributes.

However, after a calculation was completed, there was no easy way to retrieve this
dictionary with the collection of these attributes. Here we implement this method
the get_options method which by default will only return those attributes that
were explicitly set, but which can be overridden to provide the default values for
those that were not explicitly set.

The set of available options, along with their valid types, whether they are required
docstring and more, are now defined as a dictionary member of the AbstractJobCalculation
class. The JobProcess now uses this to dynamically build up the options port namespace
in its inputs specification, just like the rest of the input ports are build dynamically
based on the use methods.

@sphuber sphuber changed the title Implement the get_options method for JobCalculation [WIP] Implement the get_options method for JobCalculation Sep 12, 2018
@codecov-io
Copy link

Codecov Report

Merging #1961 into develop will decrease coverage by 0.03%.
The diff coverage is 18.75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1961      +/-   ##
===========================================
- Coverage    67.56%   67.53%   -0.04%     
===========================================
  Files          324      324              
  Lines        33342    33373      +31     
===========================================
+ Hits         22529    22537       +8     
- Misses       10813    10836      +23
Impacted Files Coverage Δ
aiida/work/process_builder.py 87.69% <0%> (ø) ⬆️
...implementation/general/calculation/job/__init__.py 42.71% <19.35%> (-0.94%) ⬇️
...orm/implementation/general/calculation/__init__.py 80.6% <0%> (+0.86%) ⬆️

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 3893c91...25c091a. Read the comment docs.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Very good for me, I was just wondering if there is a way to avoid code duplication (I was thinking to define a 'setter' and a 'getter' key-value pair inside each option, and have 'set' call through these - the only catch is that I think we still want to support the direct call of the set_* methods).

@sphuber
Copy link
Contributor Author

sphuber commented Sep 13, 2018

@giovannipizzi did you see my TODO in the PR message? Ideally we remove the explicit implementations. There are just a few with some specific logic/validation. I want to double check that this not necessary. Else we could come up with another key for the options dictionary that defines a validator function or custom getter

@giovannipizzi
Copy link
Member

Ah ok thanks, actually not, I had read the message in the email before your edit ;-)

@sphuber
Copy link
Contributor Author

sphuber commented Sep 13, 2018

And I think I just got your response about having the set method call through to the explicit ones, such that we keep those and can still be called directly. Do we really need to keep that baclwards compatibility? Given that this way of creating JobCalculations is deprecated anyway

@giovannipizzi
Copy link
Member

Well, I don't know - did we officially deprecate the old way?
Maybe at this point it's a good idea, still probably if it's deprecated we should decide if we want to keep the old way working for at least a version (then we have to keep the methods) or remove it (and then we can clean up a bit of code).
Also, possibly now the options still call through the set method?
Anyway, the set method is easy to fix (just check if the key is among the known options), the question is if we want to keep supporting the set_* and get_* methods (anyway this should also be doable modifying the __getattr__/__getattribute__ methods (I don't know if in py3 the way to do it changed). I agree that the first thing to do is to check if there are non-general things. This can be merged in the meantime, I think.

@sphuber
Copy link
Contributor Author

sphuber commented Sep 18, 2018

We didn't officially deprecate them, so I agree it is best not to remove them. I have deprecated them and point the user to the get_option and set_option methods. Our own source code I have updated to exclusively use these new methods.

@sphuber sphuber force-pushed the fix_1533_implement_jobcalculation_get_options branch from 25c091a to 7e742f1 Compare September 18, 2018 09:13
@sphuber sphuber changed the title [WIP] Implement the get_options method for JobCalculation Implement the get_options method for JobCalculation Sep 18, 2018
@sphuber sphuber force-pushed the fix_1533_implement_jobcalculation_get_options branch from 7e742f1 to b47454f Compare September 18, 2018 09:27
What is currently defined as the `options` for a `JobCalculation`, are specific
attributes that were also stored as node attributes through `set_` methods. These
were called setters and are all defined explicitly, along with the corresponding
getters. With the introduction of the `Process` paradigm, these could no longer be
called manually by the user on the calculation node, and so the `options` dictionary
was invented, to allow the user to specify these attributes.

However, after a calculation was completed, there was no easy way to retrieve this
dictionary with the collection of these attributes. Here we implement this method
the `get_options` method which by default will only return those attributes that
were explicitly set, but which can be overridden to provide the default values for
those that were not explicitly set.

The set of available options, along with their valid types, whether they are required
docstring and more, are now defined as a dictionary member of the `AbstractJobCalculation`
class. The `JobProcess` now uses this to dynamically build up the `options` port namespace
in its inputs specification, just like the rest of the input ports are build dynamically
based on the use methods.
@sphuber sphuber force-pushed the fix_1533_implement_jobcalculation_get_options branch from b47454f to 28d6d38 Compare September 18, 2018 09:38
@coveralls
Copy link

coveralls commented Sep 18, 2018

Pull Request Test Coverage Report for Build 3831

  • 40 of 103 (38.83%) changed or added relevant lines in 7 files are covered.
  • 2848 unchanged lines in 60 files lost coverage.
  • Overall coverage decreased (-2.9%) to 65.164%

Changes Missing Coverage Covered Lines Changed/Added Lines %
aiida/work/process_builder.py 0 1 0.0%
aiida/tools/dbexporters/tcod.py 0 2 0.0%
aiida/workflows/wf_XTiO3.py 0 2 0.0%
aiida/workflows/wf_demo.py 1 4 25.0%
aiida/orm/implementation/general/calculation/job/init.py 35 90 38.89%
Files with Coverage Reduction New Missed Lines %
aiida/workflows/wf_XTiO3.py 1 0.0%
aiida/common/datastructures.py 1 95.74%
aiida/control/computer.py 1 83.72%
aiida/common/ipython/ipython_magics.py 1 0.0%
aiida/orm/implementation/django/group.py 1 87.57%
aiida/orm/implementation/general/group.py 1 61.62%
aiida/restapi/common/utils.py 1 66.76%
aiida/common/extendeddicts.py 1 91.84%
aiida/orm/implementation/general/calculation/init.py 2 79.74%
aiida/orm/data/cif.py 2 80.57%
Totals Coverage Status
Change from base Build 3825: -2.9%
Covered Lines: 23201
Relevant Lines: 35604

💛 - Coveralls

@sphuber sphuber merged commit b42decb into aiidateam:develop Sep 18, 2018
@sphuber sphuber deleted the fix_1533_implement_jobcalculation_get_options branch September 18, 2018 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants