add no_copy option to NoModifier and Deprecate 'subset_mask'

[sroet]

This is a cherry picked version of 79c961e from #1075 in order to seperate out the comments

Copies of relevant comments:

@dwhswenson wrote:
More thoughts coming later, but here's a really quick one:

even with NoModification I am pretty sure new == old but not new is old (only a copy is returned).

Looking at the code, you're correct. In usage here, I think we want the default modifier (for one-way shooting, e.g.) to return new such that new is old. My suggestion (pretty easy change to the current code):

class NoModification(SnapshotModifier):
  def __init__(self, as_copy=True):
    # masking change to only certain atoms is nonsense for no modification
      super(NoModification, self).__init__(subset_mask=None)
      self.as_copy = as_copy

   def __call__(self, snapshot):
       return snapshot.copy() if self.as_copy else snapshot

Then use NoModification(as_copy=False) as the default in the changes here.

@sroet wrote:
That was way more nasty than expected, as we actually test for subset_mask on NoModification (as a proxy for subclassing SnapshotModifier) Also, apparently old.copy() != old for toy snapshots, I did not really investigate this, but I am afraid we are losing precision on the copy when seeing all the assert_almost_equal in the tests for this modifier...

@dwhswenson wrote:

I can cherry pick over that commit into a separate PR if that is preferred

Yeah, if it's going to be a headache, let's move it into a separate PR to keep discussion a little cleaner.

That was way more nasty than expected, as we actually test for subset_mask on NoModification (as a proxy for subclassing SnapshotModifier)

That part is easily fixed by subclassing in tests. I was lazy before; sorry! man_shrugging

Also, apparently old.copy() != old for toy snapshots, I did not really investigate this, but I am afraid we are losing precision on the copy when seeing all the assert_almost_equal in the tests for this modifier...

I'm not surprised by this. There's enough bouncing between C and Python representations that I wouldn't be surprised if these are only almost_equal.

[sroet]

That part is easily fixed by subclassing in tests. I was lazy before; sorry! man_shrugging

So my main question is now: I thought tested functionality was considered public API, do you want me to break it for OPS 1.x?

[codecov]

Codecov Report

Merging #1079 (967b7d7) into master (d9832f8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1079   +/-   ##
=======================================
  Coverage   81.59%   81.59%           
=======================================
  Files         140      140           
  Lines       15427    15430    +3     
=======================================
+ Hits        12587    12590    +3     
  Misses       2840     2840           

Impacted Files	Coverage Δ
openpathsampling/snapshot_modifier.py	100.00% <100.00%> (ø)

---

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 d9832f8...967b7d7. Read the comment docs.

[dwhswenson]

So my main question is now: I thought tested functionality was considered public API, do you want me to break it for OPS 1.x?

I think that was mislabeled as tested functionality. Since subset_mask has no functionality in NoModification, it can't actually do anything there. It was just me being lazy and not implementing a subclass within the tests, and not intended functionality. If anyone is affected, I'll just point to the old adage that "Every change breaks someone's workflow."

True tested functionality should be that no_modification.__call__ returns a copy by default, and now an identical object if created with as_copy=False.

[sroet]

I think that was mislabeled as tested functionality. Since subset_mask has no functionality in NoModification, it can't actually do anything there. It was just me being lazy and not implementing a subclass within the tests, and not intended functionality. If anyone is affected, I'll just point to the old adage that "Every change breaks someone's workflow."

Perfect, just wanted to make sure that is what you intended. Implemented it, please have another look

[dwhswenson]

Trivial change: One comment to either remove or at least take into consideration with regards to discussion in #1080.

[dwhswenson]

LGTM