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

Add Transformation to set box dimensions #3175

Merged
merged 49 commits into from
Apr 6, 2021
Merged

Add Transformation to set box dimensions #3175

merged 49 commits into from
Apr 6, 2021

Conversation

hp115
Copy link
Contributor

@hp115 hp115 commented Mar 16, 2021

Fixes #2691

Changes made in this Pull Request:

  • Add trajectory transformation for setting timestep dimensions

PR Checklist

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

@hp115 hp115 marked this pull request as draft March 16, 2021 20:24
@hp115 hp115 marked this pull request as ready for review March 16, 2021 20:25
@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #3175 (c50c604) into develop (62089a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c50c604 differs from pull request most recent head 6b61a59. Consider uploading reports for the commit 6b61a59 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3175   +/-   ##
========================================
  Coverage    92.73%   92.74%           
========================================
  Files          168      169    +1     
  Lines        22694    22713   +19     
  Branches      3218     3218           
========================================
+ Hits         21046    21065   +19     
  Misses        1600     1600           
  Partials        48       48           
Impacted Files Coverage Δ
package/MDAnalysis/transformations/__init__.py 100.00% <100.00%> (ø)
...ackage/MDAnalysis/transformations/boxdimensions.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 aa37b8c...6b61a59. Read the comment docs.

@tylerjereddy tylerjereddy added the Component-Transformations On-the-fly transformations label Mar 18, 2021
@pep8speaks
Copy link

pep8speaks commented Mar 19, 2021

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

Line 73:24: E127 continuation line over-indented for visual indent
Line 79:24: E127 continuation line over-indented for visual indent

Comment last updated at 2021-04-06 15:03:50 UTC

Copy link
Contributor Author

@hp115 hp115 left a comment

Choose a reason for hiding this comment

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

Thank you for reviewing @tylerjereddy, I changed my files according to your suggestions.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Hi @hp115, thank you for working on this. I think it's getting close, but I have a few comments about structuring your errors, your tests, and the documentation. Please also address the PEP8 issues noted in by the bot.

You should check that the example you put in the documentation actually works, as these typos could have been easily caught.

package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/CHANGELOG Outdated Show resolved Hide resolved
@lilyminium
Copy link
Member

Thanks @hp115, just fixed the merge conflict so tests can run.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

LGTM @IAlibay @hp115. Aside from failing build.

As a side note @IAlibay @lilyminium (not in this PR) do we want to support setting the dimensions manually to an timesteps @ [a, b, c, alpha, beta, gamma] multidimensional array. Seems like something that could be useful?

@hp115 hp115 requested a review from lilyminium April 6, 2021 08:57
Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Thanks @hp115, nearly there! As this is your first contribution to MDAnalysis, please add yourself to AUTHORS :-)

package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved
@hp115 hp115 requested a review from lilyminium April 6, 2021 14:50
@lilyminium lilyminium changed the title Issue 2691 set box dimensions Add Transformation to set box dimensions Apr 6, 2021
@lilyminium lilyminium merged commit 7618445 into MDAnalysis:develop Apr 6, 2021
@lilyminium
Copy link
Member

Thank you for adding this @hp115, it will be very useful!

@hp115
Copy link
Contributor Author

hp115 commented Apr 7, 2021

Thank you @lilyminium, @IAlibay, @hmacdope and @tylerjereddy for your help and suggestions!

@hp115 hp115 deleted the issue-2691-set-box-dimensions branch April 7, 2021 19:29
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.

Inbuilt on-the-fly transformation to set box dimensions
6 participants