Fix simtk JSON storage in SimStore

[dwhswenson]

Switch to using is_storage_iterable everywhere.

This fixes a problem that simtk.unit.Quantity objects couldn't be directly tagged (or, probably, otherwised saved as part of simulation objects).

[codecov]

Codecov Report

Merging #961 (dc4c346) into master (dc4c346) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #961   +/-   ##
=======================================
  Coverage   80.25%   80.25%           
=======================================
  Files         136      136           
  Lines       14449    14449           
=======================================
  Hits        11596    11596           
  Misses       2853     2853           

---

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 dc4c346...c7a021b. Read the comment docs.

[dwhswenson]

This is ready for review and comment. I will leave it open for at least 24 hours, merging no earlier than Mon 18 Jan 14:00 GMT (15:00 local).

[sroet]

Looked at the code, and this is definitely a cleaner implementation so +1 for that.
However, I don't see how this implementation is different from the previous one? (As it seems like a cache-wrapper around the same if-statement, so it shouldn't change the behavior?)

Do you mind adding a test-case that would illustrate the failure/fix? (also to prevent regression later on, as you definitely want to keep the ability to save simtk.unit.Quantity stuff)

[dwhswenson]

However, I don't see how this implementation is different from the previous one? (As it seems like a cache-wrapper around the same if-statement, so it shouldn't change the behavior?)

Easier to show with code. The fundamental problem is this:

>>> from simtk import unit
>>> from collections.abc import Iterable
>>> q = 2.0 * unit.nanometers
>>> isinstance(q, Iterable)
True

I would raise this as a bug, but I think Pete's probably already annoyed at me for not replying quickly on his fix for the simtk.unit bug I filed a couple weeks ago. 😆 (Issue here: isinstance(..., Iterable) checks for the presence of __iter__; simtk.unit.Quantity implements __iter__ and raises errors if the underlying thing isn't iterable.)

In any case, this is why the following things were added to SimStore and the OPS implementation of storage:

openpathsampling/openpathsampling/experimental/simstore/class_lookup.py

Lines 17 to 23 in dc4c346

	def force_true(self, cls):
	self._false_set.discard(cls)
	self._true_set.add(cls)
	def force_false(self, cls):
	self._true_set.discard(cls)
	self._false_set.add(cls)

openpathsampling/openpathsampling/experimental/storage/simtk_unit.py

Line 16 in dc4c346

is_storage_iterable.force_false(simtk.unit.Quantity)

If it actually is iterable, we still want to treat as not (the same way we do with numpy). Actually, I should probably move the numpy check into a similar setup in the class lookup: Just use force_false for that, too.

Do you mind adding a test-case that would illustrate the failure/fix? (also to prevent regression later on, as you definitely want to keep the ability to save simtk.unit.Quantity stuff)

Good point; will do.

[sroet]

openpathsampling/openpathsampling/experimental/storage/simtk_unit.py

Line 16 in dc4c346

is_storage_iterable.force_false(simtk.unit.Quantity)

Ah. I understand now how this differs from the replaced if statement (other than caching), I had not stumbled across that line when reviewing this PR.
Thanks for explaining it, I would still prefer a test to prevent regression, but other than that this looks good to me.

[dwhswenson]

Good thing that:

1. You asked for those tests, and
2. I had just relearned git diff main...branch as a way to remember what was in old unmerged branches, as well as git rev-list --left-right --count main...branch to get ahead/behind.

It turns out that I had failed to push the commit with the tests for simtk unit storage! The test you asked for is the last one in that file, and the most recent commit. But better to have the tests I wrote a month ago, too!

[sroet]

Looks mostly good to me.

One comment about the last test, as I think it is sidestepping the test due to caching, but feel free to ignore that if that is a misinterpretation of the test

[sroet]

I am not sure why codecov doesn't like it, but this looks good to me now :)

[dwhswenson]

I am not sure why codecov doesn't like it

It seems to catch up when the Py27 unit tests finish. For some reason, Py27 takes a really long time to install (I guess conda has improved performance?), so Py37 is finishing notebooks as Py27 is starting them.

We do have some tests that are a bit flaky on coverage, so I used to see a few lines (maybe 10 or so) vary in coverage. We should allow drops of 0.05% or so; not sure if I've done that or left it extremely strict.

[sroet]

It seems to catch up when the Py27 unit tests finish. For some reason, Py27 takes a really long time to install (I guess conda has improved performance?), so Py37 is finishing notebooks as Py27 is starting them.

Indeed, it seems to update when the tests finish. All green now at least :)