-
Notifications
You must be signed in to change notification settings - Fork 333
Store dimensions in JSON state as array not object since order matters #803
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
base: master
Are you sure you want to change the base?
Conversation
Since JSON dicts don't guarantee any kind of key order in the spec, this could cause issues in downstream processing of neuroglancer states Here, the dimensions are handled in a split path. One legacy path, for dicts, and a new path, as arrays. New states are saved as arrays
|
As an alternative to what is here, we could consider adding some kind of dimension order index to the original state representation. The thinking here was that the array representation more directly indicates the order dependency, but if we wanted to keep a more similar representation to what the state was then perhaps a better line is to add an index. For example with something along this kind of line you could keep the dimensions as it was, and add an additional dimension ordering to the state. |
|
In general using an object representation instead of a list makes it more concise and in some ways simpler for users to work with (albeit more difficult to reorder), and in particular works better with the JSON state editor in Neuroglancer since it will show the keys with the values collapsed. The "layers" property also used to be an object although that was changed because JavaScript does not preserve the order for keys that happen to be base 10 numbers, i.e. "0" or "1". However, arguably we should have just disallowed such layer names and preserved the object representation since it made the JSON state editor much nicer for layers. Is there a particular language or library you are trying to use that does not offer a way to preserve object key order? |
|
Note: Python>3.6 preserves key order perfectly in JavaScript also preserves key order except for keys that look like base-10 numbers. |
|
Hi Jeremy, thanks for the feedback! I'm going to move this to draft anyway since the Python side of this PR needs more work anyway on further review (the Python to_json of the state isn't correct). Can update that part soon. But to share more context about the library/language which might be of help. We're directly storing neuroglancer JSON states using django ORM https://docs.djangoproject.com/en/5.2/ref/models/fields/#django.db.models.JSONField. This uses PostgreSQL https://www.postgresql.org/docs/current/datatype-json.html Overall we were suggesting a change here because having the JSON state being reliant on object key ordering means it's harder to provide a reliable way to store and share the state in JSON format. Totally agree a lot of libraries and langauges do keep the object key ordering, just that's not a guarantee is all. Thanks for considering and sorry for the delayed response! |
Since JSON objects are unordered in the spec, this could cause issues in processing of neuroglancer states by external tools for objects stored in the state where the name order matters. For example, in something that saves states or processes states.
The primary case where this has come up is in the global viewer
dimensionswhere the order of the names determines the default order of the viewer, so the order matters. I know the precomputed annotation info metadata has a similar pattern too though.To help with this, this PR is proposing to change the dimensions to be stored in an array, not an object. Here, the dimensions are handled in a split path. One "legacy" path, for pure dictionary style objects, and a new path, which handles arrays. New states are saved as arrays in the JSON state. This happens pretty automatically in the viewer, but from Python there is an extra kind of conversion call which happens on init of a new state.
Old state dims:
New dims: