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

atomname methods can handle empty groups #4529

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

kainszs
Copy link
Contributor

@kainszs kainszs commented Mar 25, 2024

Fixes #2879

Changes made in this Pull Request:

  • the methods psi_selections(), omega_selections(), chi1_selections() can handle empty groups
  • I also noticed a fourth method which fits into the schema and also throws an error with empty groups
    • phi_selections

NOTE: Since the methods return by default a list of atom groups, the methods will also return an empty list in the case of empty residues instead of an empty residue group ask in the issue. If you accept the code for the change, I will add comments, the tests and update the CHANGELOG.

PR Checklist

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

Developers certificate of origin


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

@pep8speaks
Copy link

pep8speaks commented Mar 25, 2024

Hello @kainszs! 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 2024-03-29 20:45:57 UTC

Copy link

github-actions bot commented Mar 25, 2024

Linter Bot Results:

Hi @kainszs! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

@yuxuanzhuang
Copy link
Contributor

Hi @kainszs , welcome to MDAnalysis! Please ping me whenever you need a review for this PR.

@kainszs
Copy link
Contributor Author

kainszs commented Mar 25, 2024

Thank you for your offer to review the PR @yuxuanzhuang . What are you still missing for the review? The tests and the changelog? I'll add them tomorrow and get back to you.

@yuxuanzhuang
Copy link
Contributor

I noticed the test (main tests) all failed. It indicates something was wrong with your PR (or at least it broke something that worked before). If you click on Details, you will see which tests fail.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.64%. Comparing base (ff222fe) to head (56df581).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4529      +/-   ##
===========================================
- Coverage    93.66%   93.64%   -0.02%     
===========================================
  Files          168      180      +12     
  Lines        21240    22327    +1087     
  Branches      3913     3917       +4     
===========================================
+ Hits         19894    20908    +1014     
- Misses         888      961      +73     
  Partials       458      458              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kainszs
Copy link
Contributor Author

kainszs commented Mar 26, 2024

Hello @yuxuanzhuang, I think the PR is ready for a review.

I have noticed that some tests also fail in other PRs. In PR #4524 you mentioned that the linter and main_tests (ubuntu-latest, 3.12, true, true) may fail, but what about codecov/project?

@yuxuanzhuang
Copy link
Contributor

The codecov (coverage) also looks fine. Don't worry about the current failed checks as they also seem to be unrelated to your PR. What you also need to add to the PR is

  • add yourself to the author list :)
  • add an entry in the CHANGELOG (ref on the PR and issue)

@yuxuanzhuang
Copy link
Contributor

@kainszs The comments regarding what to be add in the changelog somehow are not showing up in this PR, but you need to add an entry in package/CHANGELOG. You need to add to the * 2.8.0 entry block; add a reference to the issue and pr (see the others as examples) and your name in line17.

@kainszs
Copy link
Contributor Author

kainszs commented Mar 27, 2024

So now I am officially an open source developer. ;)

@yuxuanzhuang thank you for your time.

@orbeckst orbeckst assigned cbouy and yuxuanzhuang and unassigned cbouy Mar 29, 2024
@orbeckst
Copy link
Member

@yuxuanzhuang given that you already commented on the PR, can you please review/shepherd the PR? If you have too much to do, please say something/suggest someone else. Thanks!

Copy link
Contributor

@yuxuanzhuang yuxuanzhuang left a comment

Choose a reason for hiding this comment

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

lgtm! please also fix the conflicts.

@orbeckst
Copy link
Member

Failures seem to be due to #4540 / #4209 so I will merge as is.

@orbeckst orbeckst merged commit a2a27aa into MDAnalysis:develop Mar 29, 2024
18 of 23 checks passed
@orbeckst
Copy link
Member

Congratulations to your first merged PR @kainszs ! 🎉

Thank you for your contribution!

@kainszs kainszs deleted the handle-empty-groups branch March 30, 2024 15:07
@yuxuanzhuang
Copy link
Contributor

Congrats! @kainszs

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.

Atomnames methods should handle empty group
5 participants