search

BICCN / cell-locator

manually align specimens to annotated 3D spaces
https://cell-locator.readthedocs.io
Other
19 stars 7 forks source link

Remove Measurements from JSON output #183

Closed allemangD closed 3 years ago

allemangD commented 3 years ago

I made these changes a few weeks ago while I was working on the conversion script, but never got to merging them into master. The changes are relevant to #182.

Cell Locator can't load a file if Measurements is None, but it can if it's not present. Since we don't use Measurements, I just remove them from the serialization output. After more discussion in the linked issue, it seems this may not be true. I'm rebuilding the same version and will do some more testing.

To shield us from similar issues in the future I've also inverted the serialization logic; rather than deleting fields we don't want, I copy the fields we do. This way if we update Slicer and inadvertently bring in new attributes to the markups output, we won't break things like this.

I've bumped the file format version number; I need to also add a converter so the broken files can be updated once we cut another release. I'll move the PR out of draft once that's done.

I've tested the new converter with the issue described in #182. All converters ignore the "measurements" value as input; the difference is in the output from each converter. Targeting version v0.1.0+2020.09.18 will unconditionally add "measurements": []. Targeting version v0.1.1+2021.06.11 will unconditionally remove the "measurements" key.

Both approaches should be compatible with the current release of Cell Locator, however since there are some unexpected problems in the #182 I think this approach makes sense. Moving forward, we don't want Cell Locator's format to be aware of the "measurements" key; however the current version should use an empty list just in case.

# remove "measurements"
python convert.py -v '0.1.0' -t '0.1.1' 'bad_file.json' 'output.json'

# set "measurements": []
python convert.py -v '0.1.0' -t '0.1.0' 'bad_file.json' 'output.json'
allemangD commented 3 years ago

After meeting with Rusty, I'm confident this won't cause any issues on the Cell-Locator side of things. In particular I think the "whitelisting" logic will help stabilize things moving forward. Merging.