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:

  • Test whether PyNWB properly carries 'unit' attribute of CurrentClampSeries.bias_current to 'unit' attribute of IZeroClampSeries.bias_current even though IZeroClampSeries.bias_current is redefined.
  • Test changes in PyNWB and ensure that PyNWB can still read older files
  • Address breaking of backward compatibility (an NWB 2.2.5 file will not validate under NWB 2.3.0 containing this change)
  • Add reference to RFC 2119 for SHOULD and RECOMMENDED.
  • Add link to BIDS documentation about using CMIXF-12 after [FIX] Recommend SI units formatting to adhere to CMIXF-12 bids-standard/bids-specification#411 is merged and BIDS documentation is updated
  • Address how to specify and validate custom / non-SI units
  • Address how strictly units should be specified and validated (recommend and have validator warn?)
  • Look into using @satra's cmixf parser https://github.com/sensein/cmixf to validate units in the NWB validator (or in https://github.com/NeurodataWithoutBorders/nwbinspector)

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