Initial Attempt at PR for issue #3603 #5013
Conversation
…e feedback as this is just a rough attempt
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5013 +/- ##
===========================================
- Coverage 93.42% 93.41% -0.01%
===========================================
Files 177 189 +12
Lines 21865 22931 +1066
Branches 3079 3079
===========================================
+ Hits 20427 21421 +994
- Misses 986 1059 +73
+ Partials 452 451 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ### Calculating the Expected Values ### | ||
|
|
||
| def calc_Prop(u, prop = 'masses'): # Property can be 'masses' or 'charges' | ||
| expected_atoms = eval(f'u.atoms.{prop}') |
There was a problem hiding this comment.
The linter (and most programmers) would heavily discourage using eval(). It could be used to run arbitrary code. Maybe you can use getattr() instead? (https://docs.python.org/3/library/functions.html#getattr)
getattr(u.atoms, prop)
There are quite a few eval() below you could replace similarly.
| expected_xcharge_residues = np.array( | ||
| [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0] | ||
| ) | ||
| test_Systems = ['neutral_Particles', 'charged_Particles', 'charged_Dimers'] |
There was a problem hiding this comment.
Instead of strings, can't you just add in the functions? You might have to move this below those functions.
| expected_xmass = [] | ||
| expected_xcharge = [] | ||
|
|
||
| for system in test_Systems: |
There was a problem hiding this comment.
With functions in test_Systems on L51, you can remove the eval() and run system(...) on L207. I would also discourage using such a generic name system for a variable, esp. since it's callable. Could mistaken with something more common like os.system.
| assert_allclose(ld.masses, expected_masses) | ||
| assert_allclose(ld.charges, expected_charges) | ||
| # rtol changed here due to floating point imprecision | ||
| assert_allclose(ld.results.x.mass_density, expected_xmass, rtol=1e-06) | ||
| assert_allclose(ld.results.x.charge_density, expected_xcharge) | ||
|
|
||
|
|
||
| for i in range(len(params)): | ||
| print(i) |
There was a problem hiding this comment.
Is this print a leftover from testing?
| def calc_Densities(u, prop = 'masses', spliceLen = 0.25): # Property can be 'masses' or 'charges' | ||
|
|
||
| propShort = 'mass' | ||
| if prop == 'charges': propShort = 'charge' |
There was a problem hiding this comment.
I would combine these two lines to:
propShort = 'charge' if prop == 'charges' else 'mass'
I don't think linters like an if statement without the actual new line/spaces
| universe = eval(f'{system}(100, 1, 5, 4)') | ||
| universes.append(universe) | ||
|
|
||
| ## expected_masses_atoms, expected_masses_residues, expected_masses_segments = calc_Prop(universe, 'masses') |
Would greatly appreciate some feedback, as this is just meant to be a rough version (+ my first time doing a PR, so, hopefully it's in the right place). So that I know, what's the best way to discuss changes and get feedback?
It's very rough at the moment, cause I have a lot of questions that I think it would be good to clear up (I think I'll try and put another message in the chat of issue #3603 summarising). I'm not expecting this to be merged immediately, and so will properly fill this out later I imagine (i.e. the checkboxes and the certification), my eyes just need a break from reading through things.
Fixes #3603
Changes made in this Pull Request:
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5013.org.readthedocs.build/en/5013/