Hbond analysis update #4107

yickysan · 2023-03-31T13:31:38Z

Fixes #3933

Changes made in this Pull Request:

Changed the guess_hydrogens, guess_donors and guess_acceptors methods to use ag.atoms instead of ag.select.atoms("all") if a selection string isn't given.

PR Checklist

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

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

…arison of arrays to use numpy.testing.assert_allclose function

github-actions · 2023-03-31T13:34:46Z

Linter Bot Results:

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

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/4582888314/jobs/8093390778

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2023-03-31T13:49:36Z

Codecov Report

Patch coverage: 61.90% and project coverage change: -0.03 ⚠️

Comparison is base (bdb1352) 93.59% compared to head (bb25dcd) 93.56%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4107      +/-   ##
===========================================
- Coverage    93.59%   93.56%   -0.03%     
===========================================
  Files          192      192              
  Lines        25133    25144      +11     
  Branches      4056     4060       +4     
===========================================
+ Hits         23523    23526       +3     
- Misses        1092     1097       +5     
- Partials       518      521       +3

Impacted Files	Coverage Δ
...DAnalysis/analysis/hydrogenbonds/hbond_analysis.py	`93.01% <61.90%> (-4.14%)`	⬇️

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.

RMeli

Hi @yickysan, thanks for you contribution. It looks like in this PR you also have the changes that you did for PR #4090. Could you please remove/revert them?

I see you are correctly working on a specific branch for this fix (yickysan:hbond-analysis-update) which is great. You probably created this branch from yickysan:numpy.testing-function-change, instead of creating it from develop and that's likely why you have both changes in this branch.

yickysan · 2023-03-31T21:21:47Z

Hi @yickysan, thanks for you contribution. It looks like in this PR you also have the changes that you did for PR #4090. Could you please remove/revert them?

I see you are correctly working on a specific branch for this fix (yickysan:hbond-analysis-update) which is great. You probably created this branch from yickysan:numpy.testing-function-change, instead of creating it from develop and that's likely why you have both changes in this branch.

Hello,
Thanks for pointing it out to me. I have deleted the numpy.testing-function-change-branch. leaving only the hbond-analysis-update.

RMeli · 2023-03-31T21:30:32Z

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

yickysan · 2023-03-31T21:33:36Z

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

0d7bfd4

9c11631

4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently reverting these commits as well.

yickysan · 2023-04-01T01:53:17Z

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

0d7bfd4

9c11631

4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

Please I need your help. In an attempt to revert the commits I have accidentally deleted some files. Anyway you can help me through this?

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).
You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

0d7bfd4

9c11631

4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently reverting these commits as well.

Hello is it possible to close this pull requests while I make another one? I think in my attempt to revert the other commits I might have broken something.

4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently re

RMeli · 2023-04-01T08:54:42Z

How did you delete the file? Were you using git revert? Since you deleted the whole testsuite/MDAnalysisTests/analysis/test_align.py file, I think the simplest fix here would be to download such file from the develop branch in the MDAnalysis repo and add it back to your repo. This way, you should have the file back, and also revert your commits regarding the file.

Don't be discouraged by these difficulties with git. They are quite normal and get better with practice: xkcd.com/1597 =)

yickysan · 2023-04-01T10:21:29Z

How did you delete the file? Were you using git revert? Since you deleted the whole testsuite/MDAnalysisTests/analysis/test_align.py file, I think the simplest fix here would be to download such file from the develop branch in the MDAnalysis repo and add it back to your repo. This way, you should have the file back, and also revert your commits regarding the file.

Don't be discouraged by these difficulties with git. They are quite normal and get better with practice: xkcd.com/1597 =)

I think I must have accidentally deleted the file trying to handle a merge conflict. I used git reset and not revert. I have redownloaded the file and added it back to my local repo.

RMeli · 2023-04-01T10:30:42Z

Make sure to commit and push, so that the file is restored in this PR as well. If everything goes well, it should disappear from the "files changed" tab.

You can read here about the difference between reset and revert: https://git-scm.com/docs/git#_reset_restore_and_revert The latter is safer because it does not change the commit history.

This reverts commit d455e4e.

…and comparison of arrays to use numpy.testing.assert_allclose function" This reverts commit f894064.

RMeli · 2023-04-01T11:11:47Z

@yickysan Looks like you removed all the changes, including the ones related to this PR? There is nothing to review at the moment. (I'm on mobile, apologies if I missed something ing.)

RMeli · 2023-04-01T11:13:47Z

You can use "git status" and "git diff" locally to check what's going and in which state is your (local) repo.

yickysan · 2023-04-01T11:23:31Z

@yickysan Looks like you removed all the changes, including the ones related to this PR? There is nothing to review at the moment. (I'm on mobile, apologies if I missed something ing.)

Yes the revert I made removed the file related to this pull request but I am adding it back right now.

yickysan · 2023-04-03T12:13:03Z

You can use "git status" and "git diff" locally to check what's going and in which state is your (local) repo.

Hello @RMeli I added back the commit related to this PR. It failed the codecov tests. Is there a way to fix this?

IAlibay

I'm not sure I understand the intent behind these changes, how is this intended to change the usability of the hbond analysis class? Maybe if you can provide a quick walked through example of how a user might help review.

re coverage: this is happening because you don't have any tests that cover the new code branches that were introduced in this PR. To avoid them, you would need to add tests that try out the new input method you've introduced.

IAlibay · 2023-04-03T13:22:51Z

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

-        ]
+        if select:
+
+            ag = self.u.select_atoms(select)


Ratther than doing a completely new code branch for each option, why not just do this selection solely in an if/else and leaving everything else the same?

yickysan · 2023-04-03T17:24:02Z

I'm not sure I understand the intent behind these changes, how is this intended to change the usability of the hbond analysis class? Maybe if you can provide a quick walked through example of how a user might help review.

re coverage: this is happening because you don't have any tests that cover the new code branches that were introduced in this PR. To avoid them, you would need to add tests that try out the new input method you've introduced.

So it doesn't change the usability of the hbondanalysis class. It just changes the guess_donors, guess_hydrogens methods, etc to call u.atoms if a selection string isn't given instead of calling u.select_atoms("all")

changed comparisons of scalars to use pytest.approx function and comp…

f894064

…arison of arrays to use numpy.testing.assert_allclose function

github-actions bot added the Component-Analysis label Mar 31, 2023

RMeli requested changes Mar 31, 2023

View reviewed changes

yickysan force-pushed the hbond-analysis-update branch 2 times, most recently from 670b9f9 to 8f8a48e Compare April 1, 2023 00:17

reverting previous commit

d455e4e

yickysan force-pushed the hbond-analysis-update branch from 8f8a48e to d455e4e Compare April 1, 2023 00:22

github-actions bot added Component-lib Component-Readers and removed Component-Readers Component-lib labels Apr 1, 2023

yickysan added 2 commits April 1, 2023 11:45

Revert "reverting previous commit"

1c49ead

This reverts commit d455e4e.

Revert "changed comparisons of scalars to use pytest.approx function …

7c98e9f

…and comparison of arrays to use numpy.testing.assert_allclose function" This reverts commit f894064.

yickysan force-pushed the hbond-analysis-update branch from 89999dd to 7c98e9f Compare April 1, 2023 10:52

github-actions bot removed the Component-Analysis label Apr 1, 2023

yickysan requested a review from RMeli April 1, 2023 10:56

hbond analysis update

bb25dcd

github-actions bot added the Component-Analysis label Apr 1, 2023

IAlibay requested changes Apr 3, 2023

View reviewed changes

yickysan closed this Apr 7, 2023

yickysan deleted the hbond-analysis-update branch April 7, 2023 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hbond analysis update #4107

Hbond analysis update #4107

yickysan commented Mar 31, 2023 •

edited

Loading

github-actions bot commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

RMeli left a comment

yickysan commented Mar 31, 2023

RMeli commented Mar 31, 2023

yickysan commented Mar 31, 2023

yickysan commented Apr 1, 2023

RMeli commented Apr 1, 2023

yickysan commented Apr 1, 2023

RMeli commented Apr 1, 2023

RMeli commented Apr 1, 2023

RMeli commented Apr 1, 2023

yickysan commented Apr 1, 2023

yickysan commented Apr 3, 2023

IAlibay left a comment

IAlibay Apr 3, 2023

yickysan commented Apr 3, 2023

Hbond analysis update #4107

Hbond analysis update #4107

Conversation

yickysan commented Mar 31, 2023 • edited Loading

PR Checklist

github-actions bot commented Mar 31, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

RMeli left a comment

Choose a reason for hiding this comment

yickysan commented Mar 31, 2023

RMeli commented Mar 31, 2023

yickysan commented Mar 31, 2023

yickysan commented Apr 1, 2023

RMeli commented Apr 1, 2023

yickysan commented Apr 1, 2023

RMeli commented Apr 1, 2023

RMeli commented Apr 1, 2023

RMeli commented Apr 1, 2023

yickysan commented Apr 1, 2023

yickysan commented Apr 3, 2023

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Apr 3, 2023

Choose a reason for hiding this comment

yickysan commented Apr 3, 2023

yickysan commented Mar 31, 2023 •

edited

Loading

github-actions bot commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading