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

Make pyedr also return a dictionary of units #56

Merged
merged 10 commits into from Aug 31, 2022

Conversation

BFedder
Copy link
Collaborator

@BFedder BFedder commented Aug 26, 2022

Following the discussion on units on PR 3749, I have changed pyedr here so it returns a dictionary of the units it reads from the EDR file as well.

@pep8speaks
Copy link

pep8speaks commented Aug 26, 2022

Hello @BFedder! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-08-31 12:21:59 UTC

@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Base: 89.21% // Head: 89.70% // Increases project coverage by +0.49% 🎉

Coverage data is based on head (8e5f566) compared to base (ccfdc89).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   89.21%   89.70%   +0.49%     
==========================================
  Files           7        7              
  Lines         482      505      +23     
==========================================
+ Hits          430      453      +23     
  Misses         52       52              
Impacted Files Coverage Δ
panedr/panedr/__init__.py 100.00% <100.00%> (ø)
panedr/panedr/panedr.py 100.00% <100.00%> (ø)
panedr/panedr/tests/test_edr.py 98.91% <100.00%> (+0.08%) ⬆️
pyedr/pyedr/__init__.py 100.00% <100.00%> (ø)
pyedr/pyedr/pyedr.py 82.72% <100.00%> (+0.38%) ⬆️
pyedr/pyedr/tests/datafiles.py 100.00% <100.00%> (ø)
pyedr/pyedr/tests/test_edr.py 96.39% <100.00%> (+0.20%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@BFedder
Copy link
Collaborator Author

BFedder commented Aug 26, 2022

I have updated this now so the units are also returned through panedr, not just pyedr

hmacdope
hmacdope previously approved these changes Aug 26, 2022
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Looks good to me @BFedder :)

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Add example of units in the readme please ?

@BFedder
Copy link
Collaborator Author

BFedder commented Aug 26, 2022

Having looked at the code again to add an example for the readme, I have realised that having the energy dict as part of the return value for edr_to_dict and edr_to_df is problematic, because people would then have a tuple at hand when they might not expect it from the function name. I have instead now written a new function, get_unit_dictionary, that returns the unit dictionary alone.

hmacdope
hmacdope previously approved these changes Aug 28, 2022
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

I like the change to a separate method. Looks good to me!

@hmacdope hmacdope requested a review from IAlibay August 30, 2022 06:31
@hmacdope
Copy link
Member

@IAlibay do you have any blocking concerns here? Otherwise I will merge so we can progress on MDAnalysis/mdanalysis#3749

@IAlibay
Copy link
Member

IAlibay commented Aug 30, 2022

I'll re-review later today, MDA release comes as a priority.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

I think get_unit_dict() does too much work — it seems it reads the whole file. Maybe I misunderstood how it works, though, then ignore.

Please also fix PEP8 complaints.

pyedr/pyedr/pyedr.py Outdated Show resolved Hide resolved
pyedr/pyedr/pyedr.py Outdated Show resolved Hide resolved
orbeckst
orbeckst previously approved these changes Aug 31, 2022
hmacdope
hmacdope previously approved these changes Aug 31, 2022
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

LGTM :)

@hmacdope
Copy link
Member

@IAlibay any outstanding comments or can I merge? :)

@IAlibay
Copy link
Member

IAlibay commented Aug 31, 2022

I'll review in a few

(EDR_BLOCKS, EDR_BLOCKS_XVG),
(Path(EDR), EDR_XVG),])
params=[(EDR, EDR_XVG, EDR_UNITS),
(EDR_IRREG, EDR_IRREG_XVG, EDR_IRREG_UNITS),
Copy link
Member

Choose a reason for hiding this comment

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

I'll let it go here because the likelihood is that no one was using these downstream, but arbitrarily renaming file object names like this is a breaking change, please avoid doing this.

return unit_dict


def edr_to_dict(path: str, verbose: bool = False) -> (Dict[str, np.ndarray],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what's going on with this PR, didn't you say that you wanted to not return a tuple here, and therefore added get_unit_dictionary, but this is still happening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really need to stop changing code without changing the documentation to match it..... thanks!

@BFedder BFedder dismissed stale reviews from hmacdope and orbeckst via d078436 August 31, 2022 12:02
@@ -436,6 +439,8 @@ def read_edr(path: str, verbose: bool = False) -> read_edr_return_type:
A list containing the names of the energy terms found in the file
times: list[float]
A list containing the time of each step/frame.
unit_dict: Dict[str, str]
Copy link
Member

Choose a reason for hiding this comment

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

This is not in the return signature?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Thanks @BFedder lgtm!

def test_output_type(self, edr):
"""
Test that the function returns a dictionary of ndarrays
"""
assert isinstance(edr.edr_dict, dict)
assert isinstance(edr.edr_dict['Time'], np.ndarray)

def test_units(self, edr):
Copy link
Member

Choose a reason for hiding this comment

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

The class name doesn't really match what this is doing, but I don't think it's worth being pedantic about it.

@orbeckst
Copy link
Member

orbeckst commented Aug 31, 2022

Are the settings on this repo that new commits dismiss reviews automatically? Is this desired? @hmacdope @IAlibay @jbarnoud @BFedder

Personally, this looks like generating too much work. I am unlikely to go back to a PR if I approved it because this means (to me at least) that all the things I cared about had been addressed. I suggest to change the settings.

@BFedder
Copy link
Collaborator Author

BFedder commented Aug 31, 2022

I don't dismiss the reviews on purpose at least (and I also find that "dismissed stale reviews" sounds a bit harsh), so I hope it happens automatically rather than by some mistake I might make. I agree with you - this should not happen automatically I think. If there are big changes still, a new review could always be requested.

@IAlibay
Copy link
Member

IAlibay commented Aug 31, 2022

@orbeckst that might be me just using default settings when I set up branch protection. This is fixed now. I assume by this that we're ok to merge?

@IAlibay IAlibay merged commit 2c0efef into MDAnalysis:master Aug 31, 2022
@orbeckst orbeckst mentioned this pull request Sep 1, 2022
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.

None yet

5 participants