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

Renamed variable asel #4077

Closed
wants to merge 1 commit into from
Closed

Conversation

HunterZ17
Copy link

@HunterZ17 HunterZ17 commented Mar 15, 2023

Fixes #3911

Changes made in this Pull Request:

  • renamed variable asel to ag_select

PR Checklist

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

📚 Documentation preview 📚: https://readthedocs-preview--4077.org.readthedocs.build/en/4077/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.19 ⚠️

Comparison is base (d49b9cb) 93.52% compared to head (5c63651) 93.33%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4077      +/-   ##
===========================================
- Coverage    93.52%   93.33%   -0.19%     
===========================================
  Files          190      191       +1     
  Lines        25028    25065      +37     
  Branches      3542     4042     +500     
===========================================
- Hits         23407    23395      -12     
- Misses        1100     1145      +45     
- Partials       521      525       +4     
Impacted Files Coverage Δ
package/MDAnalysis/analysis/rms.py 93.02% <ø> (ø)
package/MDAnalysis/coordinates/DCD.py 98.66% <100.00%> (ø)
package/MDAnalysis/coordinates/base.py 94.69% <100.00%> (ø)
package/MDAnalysis/coordinates/memory.py 96.22% <100.00%> (ø)

... and 22 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hmacdope
Copy link
Member

@MDAnalysis/coredevs what is the go here? I assume we may need to deprecate first?

@IAlibay
Copy link
Member

IAlibay commented Mar 19, 2023

@MDAnalysis/coredevs what is the go here? I assume we may need to deprecate first?

Yeah, as discussed in the original issue, we can't remove this until 3.0, so we need to deprecate the keyword for now, allow both options, and then remove in 3.0.

@hmacdope
Copy link
Member

hmacdope commented Jul 1, 2023

@HunterZ17 do you plan on working on this in the near future? If not might be best to close.

@IAlibay
Copy link
Member

IAlibay commented Jul 11, 2023

@HunterZ17 do you plan on working on this in the near future? If not might be best to close.

Closing due to lack of activity.

@IAlibay IAlibay closed this Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change name of asel in timeseries to something more comprehensible.
3 participants