Skip to content

Move results attributes of helix_analysis to Results#3279

Merged
orbeckst merged 2 commits intoMDAnalysis:developfrom
lilyminium:helix-analysis-results
May 7, 2021
Merged

Move results attributes of helix_analysis to Results#3279
orbeckst merged 2 commits intoMDAnalysis:developfrom
lilyminium:helix-analysis-results

Conversation

@lilyminium
Copy link
Member

@lilyminium lilyminium commented May 6, 2021

Fixes #3268
Fixes #3267

Changes made in this Pull Request:

  • changed versionchanged to the right number
  • Moved attributes to results

I did not add a deprecation warning because this analysis is new in 2.0.0.

PR Checklist

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

@pep8speaks
Copy link

pep8speaks commented May 6, 2021

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

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

Comment last updated at 2021-05-07 22:02:29 UTC

@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #3279 (d63d389) into develop (be3ec1b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3279   +/-   ##
========================================
  Coverage    93.04%   93.04%           
========================================
  Files          172      172           
  Lines        22751    22751           
  Branches      3192     3192           
========================================
  Hits         21168    21168           
  Misses        1533     1533           
  Partials        50       50           
Impacted Files Coverage Δ
package/MDAnalysis/analysis/helix_analysis.py 100.00% <100.00%> (ø)

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 be3ec1b...d63d389. 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.

lgtm, just suggestions how to make use of the fact that Results is a dict and get rid of ugly {get,set}attr.

@orbeckst orbeckst added this to the 2.0 milestone May 7, 2021
@orbeckst
Copy link
Member

orbeckst commented May 7, 2021

Something is not right in how the results are stored now. I am not sure what, I just looked at the failures and they are typically an extra (or missing) level of [ .. ] in the results. Maybe I made a mistake in my quick attr <-> dict translation.

@orbeckst
Copy link
Member

orbeckst commented May 7, 2021

I assume you need PR #3281 merged before restarting this one?

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.

I'll block the PR until the tests pass (which requires solving #3282 )

lilyminium and others added 2 commits May 7, 2021 15:02
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
@lilyminium lilyminium force-pushed the helix-analysis-results branch from 72a4e9c to d63d389 Compare May 7, 2021 22:02
@orbeckst orbeckst merged commit 0e7d2f9 into MDAnalysis:develop May 7, 2021
@orbeckst
Copy link
Member

orbeckst commented May 8, 2021

I am sorry, the CHANGELOG message for this PR was lost after merging PR #3288 – see comment 51fa4e4#r50540088 . This should get fixed when PR #3284 is merged.

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.

update helix_analysis to use .results Incorrect versionadded for helix_analysis?

4 participants