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 isolayer selection method #3846

Merged
merged 6 commits into from
Oct 1, 2022
Merged

Add isolayer selection method #3846

merged 6 commits into from
Oct 1, 2022

Conversation

jaclark5
Copy link
Contributor

Fixes #3845

Changes made in this Pull Request:

  • Added isolayer selection method

PR Checklist

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

@jaclark5
Copy link
Contributor Author

This selection method was used in a publication soon to be submitted.

@codecov
Copy link

codecov bot commented Sep 21, 2022 •
edited
Loading

Codecov Report

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

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

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #3846   +/-   ##
========================================
  Coverage    94.33%   94.34%           
========================================
  Files          193      193           
  Lines        24981    25007   +26     
  Branches      3370     3373    +3     
========================================
+ Hits         23567    23593   +26     
  Misses        1365     1365           
  Partials        49       49
Impacted Files Coverage Δ
package/MDAnalysis/core/groups.py 98.58% <ø> (ø)
package/MDAnalysis/core/selection.py 98.85% <100.00%> (+0.03%) ⬆️

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.

richardjgowers
richardjgowers approved these changes Sep 23, 2022
View reviewed changes
Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

This looks good to me

@richardjgowers
Copy link
Member

@jaclark5 since this PR is likely to be the first merged, can you add yourself to package/AUTHORS too?

richardjgowers
richardjgowers previously requested changes Sep 23, 2022
View reviewed changes
Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Oops, actually we've got docs for selections also here: https://github.com/MDAnalysis/mdanalysis/blob/develop/package/doc/sphinx/source/documentation_pages/selections.rst

Can you include your new selection on that doc page too?

hmacdope
hmacdope requested changes Sep 26, 2022
View reviewed changes
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 really good! Just a few quick comments.

package/doc/sphinx/source/documentation_pages/selections.rst Outdated Show resolved Hide resolved
package/doc/sphinx/source/documentation_pages/selections.rst Outdated Show resolved Hide resolved
package/MDAnalysis/core/selection.py Outdated Show resolved Hide resolved
hmacdope
hmacdope requested changes Sep 27, 2022
View reviewed changes
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.

One tiny thing then good to go.

package/CHANGELOG Outdated Show resolved Hide resolved
hmacdope
hmacdope requested changes Sep 27, 2022
View reviewed changes
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.

Sorry one more nitpick I promise.

package/CHANGELOG Show resolved Hide resolved
@jaclark5 jaclark5 marked this pull request as ready for review September 29, 2022 12:29
@richardjgowers richardjgowers dismissed their stale review September 30, 2022 17:09

go with Hugo's review

hmacdope
hmacdope approved these changes Oct 1, 2022
View reviewed changes
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.

Congrats @jaclark5! Thanks for the contribution!

@hmacdope hmacdope merged commit 5f77a54 into MDAnalysis:develop Oct 1, 2022
jaclark5 pushed a commit to jaclark5/mdanalysis that referenced this pull request Oct 1, 2022
@hmacdope @jaclark5
Add isolayer selection method (MDAnalysis#3846)
7c5b753 
* Add isolayer selection method

* Added test_isolayer assertion that comparison isn't with empty

* Added author and docs

* Isolayer: Fix documentation and removed unused code

* Update CHANGELOG

Co-authored-by: Hugo MacDermott-Opeskin <hugomacdermott@gmail.com>
@hmacdope
Copy link
Member

hmacdope commented Oct 2, 2022

@jaclark5 now that you have merged your first PR (yay) would you be able to please introduce yourself on the developer mailing list (https://groups.google.com/g/mdnalysis-devel)? This is so we have a traceable email for all authors and know a bit more about you. It is also important as we are in the slow process of re-licensing.

jaclark5 pushed a commit to jaclark5/mdanalysis that referenced this pull request Oct 11, 2022
@hmacdope @jaclark5
Add isolayer selection method (MDAnalysis#3846)
7c666f2 
* Add isolayer selection method

* Added test_isolayer assertion that comparison isn't with empty

* Added author and docs

* Isolayer: Fix documentation and removed unused code

* Update CHANGELOG

Co-authored-by: Hugo MacDermott-Opeskin <hugomacdermott@gmail.com>
@orbeckst orbeckst mentioned this pull request Nov 11, 2022
Merged
1 task
@jaclark5 jaclark5 deleted the isosurface branch November 17, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmacdope hmacdope hmacdope approved these changes

@richardjgowers richardjgowers richardjgowers left review comments

Assignees
No one assigned
Labels
Component-Core enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Isolayer selection
4 participants
@jaclark5 @richardjgowers @hmacdope @IAlibay