-
Notifications
You must be signed in to change notification settings - Fork 2
Allow 2D coordinate or mask. #58
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
Changes from all commits
2a46ff9
81e3f1a
f28ded8
e775109
b268fbc
41e0de3
f0d5dad
393e49f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Architecture Decision Records | ||
|
|
||
| ## ADR-001: Allow up to 2 dimensional coordinates/mask as coordinates and masks. | ||
|
|
||
| At the beginning we only allowed scalar or 1 dimensional coordinates or masks in the metadata. | ||
| It is because the metadata is stored as plain text in the tiff file. | ||
| If we allow arbitrary dimensional coordinates or masks, the metadata size may exceed the size of the image file itself. | ||
| There is no strict rule about the size of the metadata in the tiff format itself. | ||
| We try to keep the metadata size small as possible to increase the usability and reduce potential tourbles such as storage size or loading latency. | ||
|
|
||
| However, there was need for 2 dimensional coordinates, especially for the coordinate that depends on the pixel position. | ||
| For example, when a tiff file is a histogram of (x, y, tof), in order to compute wavelength, we need `Ltotal` of each pixel, which is a 2 dimensional (x, y) coordinate. | ||
| This usecase was found in the detector position calibration routine. | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,4 +13,5 @@ maxdepth: 2 | |
| getting-started | ||
| coding-conventions | ||
| dependency-management | ||
| adrs | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,12 +53,16 @@ | |
| "properties": { | ||
| "data": {"$ref": "#/$defs/ImageVariableMetadata"}, | ||
| "masks": { | ||
| "additionalProperties": {"$ref": "#/$defs/ScippVariable"}, | ||
| "additionalProperties": { | ||
| "anyOf": [{ "$ref": "#/$defs/ScippVariable0D" }, { "$ref": "#/$defs/ScippVariable1D" }, { "$ref": "#/$defs/ScippVariable2D" }] | ||
| }, | ||
| "title": "Masks", | ||
| "type": "object" | ||
| }, | ||
| "coords": { | ||
| "additionalProperties": {"$ref": "#/$defs/ScippVariable"}, | ||
| "additionalProperties": { | ||
| "anyOf": [{ "$ref": "#/$defs/ScippVariable0D" }, { "$ref": "#/$defs/ScippVariable1D" }, { "$ref": "#/$defs/ScippVariable2D" }] | ||
| }, | ||
| "title": "Coords", | ||
| "type": "object" | ||
| }, | ||
|
|
@@ -75,6 +79,11 @@ | |
| "ImageVariableMetadata": { | ||
| "description": "Image Metadata.", | ||
| "properties": { | ||
| "unit": { | ||
| "anyOf": [{ "type": "string" }, { "type": "null" }], | ||
| "title": "Unit" | ||
| }, | ||
| "dtype": {"title": "Dtype", "type": "string"}, | ||
| "dims": { | ||
| "maxItems": 5, | ||
| "minItems": 5, | ||
|
|
@@ -88,21 +97,22 @@ | |
| "prefixItems": [{ "type": "integer" }, { "type": "integer" }, { "type": "integer" }, { "type": "integer" }, { "type": "integer" }], | ||
| "title": "Shape", | ||
| "type": "array" | ||
| }, | ||
| "unit": { | ||
| "anyOf": [{ "type": "string" }, { "type": "null" }], | ||
| "title": "Unit" | ||
| }, | ||
| "dtype": {"title": "Dtype", "type": "string"} | ||
| } | ||
| }, | ||
| "required": ["dims", "shape", "unit", "dtype"], | ||
| "required": ["unit", "dtype", "dims", "shape"], | ||
| "title": "ImageVariableMetadata", | ||
| "type": "object" | ||
| }, | ||
| "NeutronMetadata": { | ||
| "properties": { | ||
| "neutron_type": {"$ref": "#/$defs/NeutronSourceType"}, | ||
| "wavelength_range": {"$ref": "#/$defs/ScippVariable"} | ||
| "wavelength_range": { | ||
| "maxItems": 2, | ||
| "minItems": 2, | ||
| "prefixItems": [{ "$ref": "#/$defs/ScippVariable0D" }, { "$ref": "#/$defs/ScippVariable0D" }], | ||
| "title": "Wavelength Range", | ||
| "type": "array" | ||
|
Comment on lines
+110
to
+114
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now the wavelength range has strict type of tuple[scalar, scalar] |
||
| } | ||
| }, | ||
| "required": ["neutron_type", "wavelength_range"], | ||
| "title": "NeutronMetadata", | ||
|
|
@@ -134,31 +144,89 @@ | |
| "title": "SciTiffMetadata", | ||
| "type": "object" | ||
| }, | ||
| "ScippVariable": { | ||
| "description": "Scipp Variable Metadata with the values.\n\nOnly 1D variable is allowed for metadata.", | ||
| "ScippVariable0D": { | ||
| "description": "Scipp Variable Metadata with scalar value.", | ||
| "properties": { | ||
| "unit": { | ||
| "anyOf": [{ "type": "string" }, { "type": "null" }], | ||
| "title": "Unit" | ||
| }, | ||
| "dtype": {"title": "Dtype", "type": "string"}, | ||
| "dims": {"default": [], "maxItems": 0, "minItems": 0, "title": "Dims", "type": "array"}, | ||
| "shape": {"default": [], "maxItems": 0, "minItems": 0, "title": "Shape", "type": "array"}, | ||
| "values": { | ||
| "anyOf": [{ "type": "number" }, { "type": "string" }], | ||
| "title": "Values" | ||
| } | ||
| }, | ||
| "required": ["unit", "dtype", "values"], | ||
| "title": "ScippVariable0D", | ||
| "type": "object" | ||
| }, | ||
| "ScippVariable1D": { | ||
| "description": "Scipp Variable Metadata with 1D array values.", | ||
| "properties": { | ||
| "unit": { | ||
| "anyOf": [{ "type": "string" }, { "type": "null" }], | ||
| "title": "Unit" | ||
| }, | ||
| "dtype": {"title": "Dtype", "type": "string"}, | ||
| "dims": { | ||
| "items": {"type": "string"}, | ||
| "maxItems": 1, | ||
| "minItems": 1, | ||
| "prefixItems": [{ "type": "string" }], | ||
| "title": "Dims", | ||
| "type": "array" | ||
| }, | ||
| "shape": { | ||
| "items": {"type": "integer"}, | ||
| "maxItems": 1, | ||
| "minItems": 1, | ||
| "prefixItems": [{ "type": "integer" }], | ||
| "title": "Shape", | ||
| "type": "array" | ||
| }, | ||
| "values": { | ||
| "anyOf": [{ "items": { "type": "number" }, "type": "array" }, { "items": { "type": "string" }, "type": "array" }], | ||
| "title": "Values" | ||
| } | ||
| }, | ||
| "required": ["unit", "dtype", "dims", "shape", "values"], | ||
| "title": "ScippVariable1D", | ||
| "type": "object" | ||
| }, | ||
| "ScippVariable2D": { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I followed: the 0D, 1D and 2D all seem to be identical? In which place do we actually need the different types?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I thought we would want to limit the 2D coordinate only for x, y dimension so I started splitting them into different Models so that 2D one can have literal type of dimensions but I wasn't sure if that's necessary so I removed it... I guess we can just merge all three of them now. I'll update them. |
||
| "description": "Scipp Variable Metadata with 2D array values.\n\nFor 2D array, only numbers(float/int) are allowed.", | ||
| "properties": { | ||
| "unit": { | ||
| "anyOf": [{ "type": "string" }, { "type": "null" }], | ||
| "title": "Unit" | ||
| }, | ||
| "dtype": {"title": "Dtype", "type": "string"}, | ||
| "dims": { | ||
| "maxItems": 2, | ||
| "minItems": 2, | ||
| "prefixItems": [{ "type": "string" }, { "type": "string" }], | ||
| "title": "Dims", | ||
| "type": "array" | ||
| }, | ||
| "shape": { | ||
| "maxItems": 2, | ||
| "minItems": 2, | ||
| "prefixItems": [{ "type": "integer" }, { "type": "integer" }], | ||
| "title": "Shape", | ||
| "type": "array" | ||
| }, | ||
| "values": { | ||
| "anyOf": [{ "type": "number" }, { "type": "string" }, { "items": { "type": "number" }, "type": "array" }, { "items": { "type": "string" }, "type": "array" }], | ||
| "title": "Values" | ||
| "items": { | ||
| "items": {"type": "number"}, | ||
| "type": "array" | ||
| }, | ||
| "title": "Values", | ||
| "type": "array" | ||
| } | ||
| }, | ||
| "required": ["dims", "shape", "unit", "dtype", "values"], | ||
| "title": "ScippVariable", | ||
| "required": ["unit", "dtype", "dims", "shape", "values"], | ||
| "title": "ScippVariable2D", | ||
| "type": "object" | ||
| }, | ||
| "SourceType": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| DAQMetadata, | ||
| ImageDataArrayMetadata, | ||
| ImageVariableMetadata, | ||
| ScippVariable, | ||
| ScippVariable1D, | ||
| SciTiffMetadata, | ||
| SciTiffMetadataContainer, | ||
| ) | ||
|
|
@@ -36,16 +36,16 @@ def _build_dummy_metadata() -> SciTiffMetadataContainer: | |
| unit="counts", | ||
| ), | ||
| coords={ | ||
| 't': ScippVariable( | ||
| 't': ScippVariable1D( | ||
| dims=('t',), shape=(1,), dtype='int', unit='s', values=[0] | ||
| ), | ||
| 'z': ScippVariable( | ||
| 'z': ScippVariable1D( | ||
| dims=('z',), shape=(1,), dtype='int', unit='m', values=[0] | ||
| ), | ||
| 'y': ScippVariable( | ||
| 'y': ScippVariable1D( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't x and y coords be 2d?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a dummy metadata as an example in the documentation. I didn't include 2D coordinate since we don't want to encourage people to store 2D coordinates in the metadata.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I had not seen it was creating some dummy data. |
||
| dims=('y',), shape=(1,), dtype='int', unit='m', values=[0] | ||
| ), | ||
| 'x': ScippVariable( | ||
| 'x': ScippVariable1D( | ||
| dims=('x',), shape=(1,), dtype='int', unit='m', values=[0] | ||
| ), | ||
| }, | ||
|
|
||
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.
I didn't quite get why this one is needed...