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

replaced atoms.coordinates() with atoms.positions (Issue #3467) #3477

Closed
wants to merge 3 commits into from
Closed

Conversation

pranjii
Copy link

@pranjii pranjii commented Dec 8, 2021

Fixes #3467

Changes made in this Pull Request:

PR Checklist

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

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 change: .positions is an attibute not a method.
  • Please remove the changes regarding units. We want to do 1 PR addressing 1 issue to keep things simple.

I approved running the tests – sorry, that's a current security measure. Please feel free to ping me @orbeckst if something is not moving along.

@pranjii
Copy link
Author

pranjii commented Dec 8, 2021

  • Please change: .positions is an attibute not a method.

    • Please remove the changes regarding units. We want to do 1 PR addressing 1 issue to keep things simple.

I approved running the tests – sorry, that's a current security measure. Please feel free to ping me @orbeckst if something is not moving along.

ohh sorry I am still new to github and python. I messed it up.

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #3477 (e3e6664) into develop (5bf6359) will increase coverage by 3.59%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3477      +/-   ##
===========================================
+ Coverage    90.15%   93.75%   +3.59%     
===========================================
  Files          176      176              
  Lines        23169    23169              
  Branches      3291     3297       +6     
===========================================
+ Hits         20889    21723     +834     
+ Misses        2280     1395     -885     
- Partials         0       51      +51     
Impacted Files Coverage Δ
package/MDAnalysis/analysis/encore/covariance.py 87.69% <0.00%> (ø)
package/MDAnalysis/units.py 100.00% <ø> (ø)
package/MDAnalysis/coordinates/base.py 95.47% <0.00%> (+0.11%) ⬆️
package/MDAnalysis/coordinates/TRJ.py 97.88% <0.00%> (+0.46%) ⬆️
package/MDAnalysis/lib/util.py 90.55% <0.00%> (+0.56%) ⬆️
package/MDAnalysis/core/universe.py 97.95% <0.00%> (+3.62%) ⬆️
package/MDAnalysis/analysis/align.py 98.32% <0.00%> (+5.36%) ⬆️
package/MDAnalysis/core/selection.py 98.78% <0.00%> (+5.55%) ⬆️
package/MDAnalysis/topology/guessers.py 99.22% <0.00%> (+6.97%) ⬆️
package/MDAnalysis/analysis/psa.py 89.27% <0.00%> (+10.13%) ⬆️
... and 6 more

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 5bf6359...e3e6664. Read the comment docs.

@pranjii
Copy link
Author

pranjii commented Dec 8, 2021

@orbeckst I mailed you my issue. It'd be great if you could help me out

@lilyminium
Copy link
Member

@pranjii it's usually easier, faster and helps later viewers if you discuss problems here on the issue -- then all developers can pitch in and help.

@pranjii
Copy link
Author

pranjii commented Dec 8, 2021

@pranjii it's usually easier, faster and helps later viewers if you discuss problems here on the issue -- then all developers can pitch in and help.

As I said I am new to github so I do not know the norms and I apologise for the troubles. I was not aware of how github worked and made changes to the already committed code unaware that any changes I made to my branch will also be made in the PR. So how do I reverse these changes keeping in mind that I made a small change in the code directly through github? Should I just close my pull request?

@pranjii pranjii closed this Dec 8, 2021
@orbeckst
Copy link
Member

orbeckst commented Dec 8, 2021

Hi @pranjii , don't worry – you're fine!!

There was no need to delete anything. I would have asked you to

  • fix your code in your branch & push
  • continue working on this PR

It's totally normal that you will change and rewrite the code in the PR in response to comments. It's actually really good to see your code so that we can comment early.

In short: you did everything correctly!

@pranjii
Copy link
Author

pranjii commented Dec 8, 2021

@orbeckst thank u. I have closed this PR and will change the recommend things ASAP for a new PR and guess will dive a little bit deeper into git and github

@orbeckst
Copy link
Member

orbeckst commented Dec 8, 2021

Sure. Don't worry, learning the whole pull request/review workflow takes some time. Ask us before you delete anything, we can typically give guidance. Just be aware that we might not always be able to immediately respond — we are all pretty busy an it can sometimes take a day. As @lilyminium said, we discuss all issues in public. We also follow our Code of Conduct in conducting all these discussions respectfully.

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.

replace atoms.coordinates() with atoms.positions
3 participants