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

Periodic Angle Padding #242

Merged
merged 6 commits into from
Mar 27, 2023
Merged

Periodic Angle Padding #242

merged 6 commits into from
Mar 27, 2023

Conversation

@cadeduckworth cadeduckworth changed the title first commit, proposed new method Periodic Angle Padding Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #242 (adede9a) into develop (ecab235) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #242   +/-   ##
========================================
  Coverage    79.96%   79.96%           
========================================
  Files           13       13           
  Lines         1847     1847           
  Branches       282      282           
========================================
  Hits          1477     1477           
  Misses         280      280           
  Partials        90       90           
Impacted Files Coverage Δ
mdpow/workflows/dihedrals.py 93.22% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cadeduckworth cadeduckworth requested a review from orbeckst March 2, 2023 07:47
@cadeduckworth
Copy link
Contributor Author

We were getting the same results with the older method, so I do not think any tests or testing values need to be changed.

@cadeduckworth
Copy link
Contributor Author

cadeduckworth commented Mar 21, 2023

Below is the gist/notebook showing the implementation of the .copy() method used in individual steps with examples and quick tests:

https://gist.github.com/cadeduckworth/58e687f93a3612cf680c660f3181f13e

EDIT: @orbeckst

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.

Please also update CHANGES.

mdpow/workflows/dihedrals.py Outdated Show resolved Hide resolved
Comment on lines +422 to +424
df1 = df[df.dihedral > 180 - padding].copy(deep=True)
df1.dihedral -= 360
df2 = df[df.dihedral < -180 + padding]
df2 = df[df.dihedral < -180 + padding].copy(deep=True)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be ok to leave out deep=True and rely on the default. I understand that you're trying to be extra cautious. Your choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to leave it to be cautious and explicit, but also so that someone working on this module in the future is aware that this specification could be necessary in the event that pandas makes any changes.

@cadeduckworth
Copy link
Contributor Author

@orbeckst
requested changes addressed

@orbeckst orbeckst self-assigned this Mar 27, 2023
@orbeckst orbeckst merged commit b7d0b06 into develop Mar 27, 2023
@orbeckst orbeckst deleted the periodic-angle branch March 27, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants