Skip to content

Added space to separate field in output files.#2733

Closed
rcrehuet wants to merge 1 commit intoMDAnalysis:developfrom
rcrehuet:develop
Closed

Added space to separate field in output files.#2733
rcrehuet wants to merge 1 commit intoMDAnalysis:developfrom
rcrehuet:develop

Conversation

@rcrehuet
Copy link
Contributor

The output files had all the the floating point numbers joined in a single string.
I added a whitespace separator. I may have added some unnecessary whitespaces, but I don't think that creates any problem. In other words, I did not check that all added whitespeces where needed, some may go at the end of the line, where they are superflous but not problematic.

@RMeli
Copy link
Member

RMeli commented Jun 10, 2020

Hi @rcrehuet, FYI I think helanal.py is getting deprecated soon and a new module will be taking its place (see #2622). @lilyminium might be able to give more details here.

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.

@rcrehuet thank you for the contribution!

LGTM – we didn't really define the output files in any rigorous manner (this is very old code from the early days) and adding the spaces makes sense when the format strings overflow.

The module is being deprecated in favor of #2622 but give that people might still want to use the old version I am ok with fixing it.

@lilyminium @fiona-naughton do you have any comments?

Otherwise: @rcrehuet please add entries to

  • CHANGELOG
  • AUTHORS

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #2733 into develop will not change coverage.
The diff coverage is 92.85%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2733   +/-   ##
========================================
  Coverage    91.22%   91.22%           
========================================
  Files          176      176           
  Lines        24115    24115           
  Branches      3160     3160           
========================================
  Hits         22000    22000           
  Misses        1492     1492           
  Partials       623      623           
Impacted Files Coverage Δ
package/MDAnalysis/analysis/helanal.py 87.93% <92.85%> (ø)

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 6eb8f68...795a5b7. Read the comment docs.

@lilyminium
Copy link
Member

I can't remember the details anymore but #2622 does alter the algorithm, e.g. it safeguards helix height so it can no longer be negative, and the screw angles are now different. It is currently stalled as @fiona-naughton and I have been working off different interpretations of what a screw angle is -- outside opinions are very welcome -- but otherwise nearly there.

@orbeckst orbeckst self-assigned this Jun 13, 2020
@fiona-naughton
Copy link
Contributor

Progress with #2622 has unstalled for now; but this does make sense if we're keeping the old helanal around.

There's also an issue where ref_axis is never actually passed to the function that calculates the helix tilt angles (vector_of_best_fit) - the z-axis is always used by defalt. Wouldn't take much to fix, so that could be done here too?

@orbeckst
Copy link
Member

We can back-port it as a fix in #2798 .

All I'd need here are additions to CHANGELOG and AUTHORS.

@orbeckst orbeckst added this to the 1.0.x milestone Jun 27, 2020
orbeckst pushed a commit that referenced this pull request Aug 27, 2020
- fixes The output files had all the the floating point numbers joined in a single string.
- see PR #2733
- update AUTHORS
- update CHANGELOG
@orbeckst
Copy link
Member

I am including the patch 8f729d8 as a backport for 1.0.1 in PR #2798 and I added @rcrehuet to the AUTHORS file.

Thank you!

@orbeckst orbeckst closed this Aug 27, 2020
orbeckst pushed a commit that referenced this pull request Aug 27, 2020
- fixes The output files had all the the floating point numbers joined in a single string.
- see PR #2733
- update AUTHORS
- update CHANGELOG
orbeckst pushed a commit that referenced this pull request Aug 27, 2020
- fixes The output files had all the the floating point numbers joined in a single string.
- see PR #2733
- update AUTHORS
- update CHANGELOG
orbeckst pushed a commit that referenced this pull request Aug 27, 2020
- fixes The output files had all the the floating point numbers joined in a single string.
- see PR #2733
- update AUTHORS
- update CHANGELOG
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.

5 participants