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 timeseries for all readers #3890

Merged
merged 21 commits into from
Nov 11, 2022

Conversation

hmacdope
Copy link
Member

@hmacdope hmacdope commented Oct 28, 2022

Fixes #3881 #2271 #3898 #2948 MDAnalysis/mdaencore#30

Changes made in this Pull Request:

  • Add timeseries attribute to all readers as a method of ReaderBase
  • Make MemoryReader raise a NoDataError on empty atomgroup.

@MDAnalysis/coredevs one issue to be discussed. The MemoryReader has inclusive start, stop, step which is not in line with python range while this has PR been set up to have non-inclusive start-stop-step. Which would we prefer? One of the two implementations will need to change for consistency.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@hmacdope hmacdope requested a review from orbeckst October 28, 2022 01:56
@hmacdope hmacdope self-assigned this Oct 28, 2022
@hmacdope hmacdope added Component-Readers CZI-performance performance track of CZIEOSS4 grant labels Oct 28, 2022
@orbeckst
Copy link
Member

Let’s be consistent with Python range.

@hmacdope
Copy link
Member Author

Let’s be consistent with Python range.

@orbeckst to be clear this will mean making API break for memoryreader. Are we happy with that?

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.

mostly reformatting to avoid the if/else tree

package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/test_reader_api.py Outdated Show resolved Hide resolved
@IAlibay
Copy link
Member

IAlibay commented Oct 28, 2022

I'd be accepting of calling the MemoryReader weird indexing a bug rather than a feature, unless it's documented as inclusive. If it is documented to be a given way, then we'll need to do the whole deprecation warning + allow for both options specifically for the MemoryReader.

@orbeckst
Copy link
Member

Unfortunately, MemoryReader.timeseries is documented as start and stop as inclusive https://docs.mdanalysis.org/stable/documentation_pages/coordinates/memory.html?highlight=memoryreader#MDAnalysis.coordinates.memory.MemoryReader.timeseries , this is probably a remnant of the very earliest versions of timeseries that was linked closely to the old DCDReader (even though DCDReader.timeseries now contains exclusive stop).

Let’s deprecate the behavior for MemoryReader and change in 3.0. Make everything else consistent and range-like.

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.

Let’s deprecate the behavior for MemoryReader and change in 3.0. Make everything else consistent and range-like.

@hmacdope
Copy link
Member Author

I will PR deprecating the indexing of the memory reader separately.

@hmacdope
Copy link
Member Author

Let’s deprecate the behavior for MemoryReader and change in 3.0. Make everything else consistent and range-like.

Issue raised #3893

@hmacdope hmacdope requested review from orbeckst and IAlibay October 31, 2022 23:41
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.

couple of small things

package/MDAnalysis/coordinates/base.py Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/base.py Show resolved Hide resolved
package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Base: 94.35% // Head: 94.36% // Increases project coverage by +0.01% 🎉

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    MDAnalysis/mdanalysis#3890      +/-   ##
===========================================
+ Coverage    94.35%   94.36%   +0.01%     
===========================================
  Files          194      194              
  Lines        25060    25109      +49     
  Branches      3392     3398       +6     
===========================================
+ Hits         23645    23694      +49     
  Misses        1366     1366              
  Partials        49       49              
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/DCD.py 100.00% <100.00%> (ø)
package/MDAnalysis/coordinates/base.py 94.60% <100.00%> (+0.23%) ⬆️
package/MDAnalysis/coordinates/memory.py 98.75% <100.00%> (+0.01%) ⬆️
MDAnalysisTests/coordinates/base.py 94.25% <0.00%> (+0.25%) ⬆️

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.

@hmacdope
Copy link
Member Author

hmacdope commented Nov 1, 2022

I have added an exception on empty atomgroup for MemoryReader.timeseries(asel) in line with the DCDReader. Feel free to discuss on #3898.

@hmacdope hmacdope requested a review from IAlibay November 1, 2022 06:08
@hmacdope hmacdope requested a review from IAlibay November 3, 2022 01:03
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.

Just temp blocking as a follow up from #3898, the folks that have the stronger stance on the use of NoDataError probably will have strong views.

package/MDAnalysis/coordinates/base.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
@hmacdope hmacdope requested a review from IAlibay November 6, 2022 03:49
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.

Just two changelog things - I'm going to approve so that this can be merged once those comments are addressed.

package/CHANGELOG Outdated Show resolved Hide resolved
package/CHANGELOG Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/base.py Show resolved Hide resolved
Co-authored-by: Irfan Alibay <[email protected]>
@hmacdope
Copy link
Member Author

hmacdope commented Nov 6, 2022

@orbeckst this should be good to roll if you have a second to review? 😺

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.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component-Readers CZI-performance performance track of CZIEOSS4 grant enhancement
Projects
3 participants