Skip to content

Switch GNMAnalysis to AnalysisBase#3244

Merged
orbeckst merged 10 commits intoMDAnalysis:developfrom
PicoCentauri:3243-switch-GNMAnalysis-to-AnalysisBase
Apr 28, 2021
Merged

Switch GNMAnalysis to AnalysisBase#3244
orbeckst merged 10 commits intoMDAnalysis:developfrom
PicoCentauri:3243-switch-GNMAnalysis-to-AnalysisBase

Conversation

@PicoCentauri
Copy link
Contributor

Fixes #3243

Changes made in this Pull Request:
Switch GNMAnalysis to AnalysisBase.

I don't think a versionchanged info is necessary here. For the user there are no breaking changes.

PR Checklist

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

@pep8speaks
Copy link

pep8speaks commented Apr 26, 2021

Hello @PicoCentauri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 140:80: E501 line too long (80 > 79 characters)

Comment last updated at 2021-04-28 11:41:50 UTC

@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #3244 (c09bec2) into develop (a23b1e9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3244      +/-   ##
===========================================
+ Coverage    92.83%   92.84%   +0.01%     
===========================================
  Files          170      170              
  Lines        22809    22806       -3     
  Branches      3242     3241       -1     
===========================================
  Hits         21174    21174              
+ Misses        1587     1584       -3     
  Partials        48       48              
Impacted Files Coverage Δ
package/MDAnalysis/analysis/gnm.py 99.18% <100.00%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a23b1e9...c09bec2. Read the comment docs.

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.

This looks like an uncontroversial, easy LGTM to me.

I looked at the 80% coverage for the diff and I don't think it's an issue. The only thing that's not tested is the case when the SVD fails and we skip a frame with an ugly print message.

Thanks @PicoCentauri .

@IAlibay would it be ok to squeeze it into 2.0? I'll add it to the milestone but leave it to you to remove it or to merge.

@orbeckst orbeckst added this to the 2.0 milestone Apr 27, 2021
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.

Couple of comments, I realise the print statement is a leftover of the previous code, but it would be much better if we could easily test it.

@orbeckst if one of the coredevs is happy to ensure that this gets merged on time for the 2.0 deadline then I have no objections. My comment on discord were more intented as a "please prioritize reviewing efforts on what is on the current to-do list" rather than a blanket code freeze.

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.

Couple final comments (docs + matching on the warning text).

PicoCentauri and others added 3 commits April 28, 2021 08:33
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
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.

Docs do indeed build weirdly, looks like just a missing empty line though.

PicoCentauri and others added 2 commits April 28, 2021 13:40
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
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 - @orbeckst are you happy with the recent changes?

@orbeckst
Copy link
Member

orbeckst commented Apr 28, 2021 via email

@orbeckst orbeckst self-assigned this Apr 28, 2021
@orbeckst
Copy link
Member

@IAlibay I'm jumping in and merging it — I think we're all happy with the state of the PR.

@orbeckst orbeckst merged commit 4753a8c into MDAnalysis:develop Apr 28, 2021
@PicoCentauri PicoCentauri deleted the 3243-switch-GNMAnalysis-to-AnalysisBase branch June 5, 2024 15:54
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.

Switch GNMAnalysis to AnalysisBase.

4 participants