-
Notifications
You must be signed in to change notification settings - Fork 720
Initial Attempt at PR for issue #3603 #5013
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
Open
EchoFia
wants to merge
9
commits into
MDAnalysis:develop
Choose a base branch
from
EchoFia:new-testLinearDensity
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+282
−111
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
86462f8
Initial Attempt at PR for issue #3603, would appreciate some feedback…
09a9f60
Fixed pytest and Centre of Mass for issue #3603
920c4df
Fixed pytest and Centre of Mass for issue #3603
2de699e
Updated AUTHORS & CHANGELOG, Improved Formatting
f906031
Updated AUTHORS & CHANGELOG, Improved Formatting
661638a
Updated AUTHORS & CHANGELOG, Improved Formatting
73e586d
Updated AUTHORS & CHANGELOG, Improved Formatting
9b04325
Updated package/AUTHORS
b541d3f
Merge branch 'develop' into new-testLinearDensity
EchoFia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't delete existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! So, my understanding of the problem was that the test for lineardensity.py was using itself to check itself (i.e. the saved 'answers' to the test were generated using the function) and that the test cases didn't cover the idea of charged residues (or larger) as water is neutral.
As such, I used the code given that creates a universe of 100 atoms, modified it such that the charges + positions matched the 3 different types of systems (neutral particles, charged particles & charged dimers), and then calculated the values of (total masses, total charges, mass densities and charge densities) for atoms, residues and segments. I then edited the pytest function to compare these to the values generated by LinearDensity.
Let me know if you want me to explain further, I feel that the code itself is simple enough to read through (though maybe that's because I wrote it, so I might be tricking myself), so I assume the uncertainty comes in what I'm trying to achieve, which, I've hopefully explained. I had a lot of questions at the start which I put in issue #3603, but those haven't been seen yet so I interpreted it as best I could.
Apologies for removing the tests with the water system, I can add it back, updating it for how I've restructured the test (essentially, just adding in the universe for the test, which won't change anything for the water system). I can also go through and change the spellings to American English.
In regards to the comments about naming things 'make_universe' as opposed to 'make_Universe', should I change that or leave it? From looking at other code, it appeared that the standard thing done in the module was capitalised after the first word with underscores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, whilst I have your attention, can I clarify if my GSoC proposal is still valid or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what do you mean by "valid"?
(I also commented on your question #5015 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that for a valid GSoC application to MDAnalysis, you needed:
and I had assumed that the PR had to be merged by the 8th as well, with the proposal, so I was worried that I'd missed that deadline. Thanks for answering question #5015, that has clarified things, and hopefully I'll be able to get this PR merged before you make assessments.