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

Add Outputs base class #29

Merged
merged 13 commits into from
Apr 2, 2024
Merged

Add Outputs base class #29

merged 13 commits into from
Apr 2, 2024

Conversation

JosePizarro3
Copy link
Collaborator

Adding base class for outputs and the unit testing. Testing is a bit empty, but I guess we will see more once we implement other cases.

@JosePizarro3 JosePizarro3 self-assigned this Apr 2, 2024
@JosePizarro3 JosePizarro3 linked an issue Apr 2, 2024 that may be closed by this pull request
@JosePizarro3
Copy link
Collaborator Author

(please, disregard the failing pipeline; tests are passing locally)

src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Show resolved Hide resolved
Copy link
Collaborator

@ndaelman-hu ndaelman-hu left a comment

Choose a reason for hiding this comment

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

Just some minor points, but I agree with most of the approach.

src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
Comment on lines 97 to 105
def check_is_derived(self, is_derived: bool, outputs_ref) -> Optional[bool]:
if not is_derived:
if outputs_ref is not None:
return True
return False
elif is_derived and outputs_ref is not None:
return True
return None

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I follow the logic here (maybe also add a small pydocs):

This function is used to set is_derived, correct?
If is_derived is already set (i.e. is not None), only then it should trigger.

Moreover, if outputs_ref fully covers is_derived, then you don't need the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One more point, I'm going to ask Area D for "write-protected" quantities, i.e. if the value has been set, it can't be updated. This would help make the normalization leaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, because it might be missing outputs_ref, hence the None return describes the "missing refs" case. I added a better description after you reviewed it, sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, but let's run over the possible scenarios:

  1. both outputs_ref and is_derived are set, no issue
  2. only outputs_ref is set: you can set is_derived = True, but no additional information is conveyed
  3. is_derived = True and outputs_ref = None: when would this happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For number 3., maybe some wrong setting of is_derived or some missing outputs_ref when normalizing? In this case, return None is happening

src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
tests/test_outputs.py Outdated Show resolved Hide resolved
@JosePizarro3
Copy link
Collaborator Author

Just finished some first version, feel free to do another review.

You can also resolve threads when you feel you satisfied with the resolutions.

@JosePizarro3 JosePizarro3 force-pushed the 27-add-outputs-base-class branch from 44bfa2c to 4ef24ea Compare April 2, 2024 12:52
pyproject.toml Show resolved Hide resolved
@JosePizarro3 JosePizarro3 force-pushed the 27-add-outputs-base-class branch from 4ef24ea to 415c685 Compare April 2, 2024 13:01
@coveralls
Copy link

coveralls commented Apr 2, 2024

Pull Request Test Coverage Report for Build 8523567766

Details

  • 23 of 23 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+21.9%) to 88.571%

Totals Coverage Status
Change from base Build 8277368865: 21.9%
Covered Lines: 31
Relevant Lines: 35

💛 - Coveralls

@JosePizarro3 JosePizarro3 merged commit 118a8b5 into develop Apr 2, 2024
4 checks passed
@JosePizarro3 JosePizarro3 deleted the 27-add-outputs-base-class branch April 2, 2024 13:28
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.

Add Outputs base class
4 participants