-
Notifications
You must be signed in to change notification settings - Fork 1
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
Units #11
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR includes a lot of different features. In the future can you try to keep the docs changes separate from the schema changes? It would also be good to add more to the PR description, eg a summary of breaking schema changes.
This looks good other than the e/Ų
unit.
Eventually I think we might want to define QuantityValue
more precisely using qudt, but I think that can be backwards compatible with the current definition.
There's a few issues with the generated preview, but let's merge this and then fix the docs afterwards.
"acquisition": { | ||
"detector": "Falcon 4i", | ||
"dose_per_movie": { | ||
"unit": "e/Ų", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we decide whether to use electrons/Å^2
or Å^-2
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we decided on Å^-2 as electrons are not a unit and to avoid confusion with the scicat SI converter. The fact the count concerns electrons is explicitly specified in the schemas field description.
Should now properly support the documentation website, have namespaces and clear the tests without issues.