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

Array copy bugfix #126

Merged
merged 3 commits into from Apr 30, 2018
Merged

Array copy bugfix #126

merged 3 commits into from Apr 30, 2018

Conversation

bas-rustenburg
Copy link
Member

This uses deepcopy for numpy arrays used inside attempt_state_change. Potentially the main cause of #124

A short description of the problem:

If you index a list in python with my_list[:], this returns a copy of the entire list. For numpy arrays,
it returns a reference to the array. This leads to unexpected side effects when copying is intended. I've replaced array slices with deepcopy in several places in the code where copy was the intended behavior.

Bas Rustenburg added 3 commits April 30, 2018 13:58
Why:
This prevents cache files from accidentally getting checked in.

This change addresses the need by:

*  add .mypy_cache to gitignore
* add .pytest_cache to gitignore
Why:

This is a bug that is potentially responsible for
the accumulation of ions we have observed in
ion swapping simulations. In short:
using slice notations ( listname[:]) returns
a copy for python lists, but for ndarrays, it
returns a reference. We should explicitly make a deep copy instead.

This change addresses the need by:

*  Make copies of a swapper.stateVector in
NCMCProtonDrive.attempt_state_change
Why:

Numpy array slices return references, not copies.
To prevent unintended side effects, I've changed some variable assignments to use
deepcopy instead of slices.

This change addresses the need by:

replace array slices by deepcopy in
* ncmcreporter
* samsreporter
* titrationreporter
@jchodera
Copy link
Member

You've probably gone a little overkill here, but there shouldn't be any issues with this. Thanks for fixing this!

@codecov-io
Copy link

Codecov Report

Merging #126 into master will increase coverage by 0.02%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   59.65%   59.68%   +0.02%     
==========================================
  Files          36       36              
  Lines        5109     5112       +3     
==========================================
+ Hits         3048     3051       +3     
  Misses       2061     2061
Impacted Files Coverage Δ
protons/app/driver.py 85.49% <100%> (ø) ⬆️
protons/app/samsreporter.py 95.5% <100%> (+0.05%) ⬆️
protons/app/ncmcreporter.py 77.35% <50%> (+0.21%) ⬆️
protons/app/titrationreporter.py 85.18% <66.66%> (+0.13%) ⬆️

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 1da1678...b1489e1. Read the comment docs.

@bas-rustenburg bas-rustenburg merged commit d37a4dd into master Apr 30, 2018
@bas-rustenburg bas-rustenburg deleted the array-copy-fix branch April 30, 2018 19:02
@bas-rustenburg
Copy link
Member Author

I'm going to rerun those ion simulations and see if this fixes the accumulation bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants