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

[WIP] Standardize units of measurement to use CMIXF-12 #446

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

rly
Copy link
Contributor

@rly rly commented May 30, 2020

Replaces #324. See initial discussion and suggestions there.

Also fix #410.

TODO:

Copy link
Contributor

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wondered if validator could verify that unit it finds in spec field (and extensions) is confirmant to cmixf-12?

2.3.0 (Upcoming)
----------------------

- All "unit" fields representing units of measurement now follow the CMIXF-12 format as described in:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially code breaking change, needs more prominent notice. May be worth creating sectioning like in https://github.com/datalad/datalad/blob/master/CHANGELOG.md#0126-april-23-2020----

@rly rly added the compatibility: breaking change fixes or enhancements that will break schema compatability label Jun 1, 2020
@bendichter
Copy link
Contributor

@rly this looks good. Are you still interested in making these changes?

@t-b
Copy link
Contributor

t-b commented Dec 1, 2022

As long as

Test changes in PyNWB and ensure that PyNWB can still read older files

is met I'm very much in favour of this change. Is there a more official version of CMIXF-12 available? A user website from an university doesn't look that future proof.

@yarikoptic
Copy link
Contributor

although generally I would agree (and there is even no wikipedia page, I've initiated a draft https://en.wikipedia.org/wiki/Draft:CMIXF-12 -- please join to finalize/submit ), given the age of it (last released in 2011-10-09 with development from 2001), and current year (2022) -- I think there is good evidence that it would persist going forward too. But indeed the author (or us) might want to ensure its presence/maintainability going forward (hence Wiki page to start with). FWIW it is also archived by the internet archive: https://web.archive.org/web/20221017164452/https://people.csail.mit.edu/jaffer/MIXF/CMIXF-12

@satra
Copy link

satra commented Dec 1, 2022

we can also add a copy to the parser (https://github.com/sensein/cmixf) and then store it on zenodo by minting a release.

@CodyCBakerPhD
Copy link
Contributor

If hard-baking it into core NWB is deemed too extreme or would break too much backward compatibility, also see the proposal to make this a 'Best Practice' checked during NWB Inspection (and DANDI validate by extension): NeurodataWithoutBorders/nwbinspector#208 & NeurodataWithoutBorders/nwbinspector#281

@oruebel
Copy link
Contributor

oruebel commented Dec 5, 2022

... also see the proposal to make this a 'Best Practice' checked during NWB Inspection (and DANDI validate by extension): NeurodataWithoutBorders/nwbinspector#208 & NeurodataWithoutBorders/nwbinspector#281

Making this a best practice is certainly a good idea. However, the best practice really only works for fields that are user-defined, i.e., unit values that are fixed in the schema would need to be ignored.

If hard-baking it into core NWB is deemed too extreme or would break too much backward compatibility, ...

In principal, I think this should be feasible, mainly because this modified only fixed values of attributes and does not change the structure of the schema (i.e., it does not add or remove any existing fields). I don't think the values of attributes are being enforced on read, so I don't think this should cause any issues in the APIs on read, and if they are checked then migrating to the new values should be possible, since this only change the syntax of the units not the actual type of unit.

@rly thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility: breaking change fixes or enhancements that will break schema compatability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

icephys.yaml: Some entries miss unit attribute
8 participants