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.

Sign up for GitHub

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

Jump to bottom

Add deprecation warning for memoryreader inclusive indexing in timeseries. #3894

Merged
merged 11 commits into from
Nov 1, 2022
Merged

Add deprecation warning for memoryreader inclusive indexing in timeseries. #3894

merged 11 commits into from
Nov 1, 2022

Conversation

hmacdope
Copy link
Member

Related to #3893
Changes made in this Pull Request:

  • Adds deprecation warning for memoryreader.

PR Checklist

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

@hmacdope hmacdope added the CZI-performance performance track of CZIEOSS4 grant label Oct 29, 2022
@hmacdope hmacdope added this to the Release 3.0 milestone Oct 29, 2022
@hmacdope hmacdope self-assigned this Oct 29, 2022
@hmacdope hmacdope marked this pull request as ready for review October 29, 2022 01:27
@codecov
Copy link

codecov bot commented Oct 29, 2022 •
edited
Loading

Codecov Report

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

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

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #3894   +/-   ##
========================================
  Coverage    94.35%   94.35%           
========================================
  Files          194      194           
  Lines        25064    25066    +2     
  Branches      3391     3392    +1     
========================================
+ Hits         23649    23651    +2     
  Misses        1366     1366           
  Partials        49       49
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/memory.py 98.74% <100.00%> (+0.01%) ⬆️

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 hmacdope changed the title Add deprecation warning for memoryreader inclusive indexing. Add deprecation warning for memoryreader inclusive indexing in timeseries. Oct 29, 2022
orbeckst
orbeckst requested changes Oct 29, 2022
View reviewed changes
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 suggest to make the warning more specific and we need

  • test
  • entry in CHANGELOG under Deprecations
  • doc entry for deprecation (under stop keyword)

package/MDAnalysis/coordinates/memory.py Outdated
@@ -515,6 +515,10 @@ def timeseries(self, asel=None, start=0, stop=-1, step=1, order='afc'):
.. versionchanged:: 1.0.0
Deprecated `format` keyword has been removed. Use `order` instead.
"""
warnings.warn("MemoryReader.timeseries inclusive `start` and `stop`"
Copy link
Member

Choose a reason for hiding this comment

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

Can we do the warning only if stop is changed and not the default?

The warning should only be about start becoming exclusive instead of inclusive.

Copy link
Member Author

Choose a reason for hiding this comment

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

@orbeckst can warn only if stop is set.

I assume you mean stop rather than start in "The warning should only be about start becoming exclusive instead of inclusive"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you’re right, of course. Sorry for the typo.

@hmacdope hmacdope requested a review from orbeckst October 30, 2022 11:29
IAlibay
IAlibay requested changes Oct 30, 2022
View reviewed changes
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
orbeckst
orbeckst requested changes Oct 30, 2022
View reviewed changes
package/CHANGELOG Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
@hmacdope hmacdope requested review from orbeckst and IAlibay October 31, 2022 23:34
IAlibay
IAlibay requested changes Oct 31, 2022
View reviewed changes
package/MDAnalysis/coordinates/memory.py Show resolved Hide resolved
package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
IAlibay
IAlibay requested changes Nov 1, 2022
View reviewed changes
Copy link
Member

@IAlibay IAlibay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See edit: oops I started writing here then wrote the specific comment

package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
orbeckst
orbeckst approved these changes Nov 1, 2022
View reviewed changes
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

(just applying suggestions)

package/MDAnalysis/coordinates/memory.py Outdated Show resolved Hide resolved
@orbeckst @IAlibay
Apply suggestions from code review
3527825 
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
IAlibay
IAlibay approved these changes Nov 1, 2022
View reviewed changes
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.

lgtm assuming CI comes back green

@IAlibay IAlibay merged commit db2b5e4 into MDAnalysis:develop Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IAlibay IAlibay IAlibay approved these changes

@orbeckst orbeckst orbeckst approved these changes

Assignees

@hmacdope hmacdope

Labels
Component-Readers CZI-performance performance track of CZIEOSS4 grant Format-MemoryReader maintainability
Projects
CZI Performance track
Status: Done
Milestone
Release 3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@hmacdope @orbeckst @IAlibay