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

Specification of Column units attribute values? #125

Open
bmaranville opened this issue Feb 26, 2024 · 2 comments
Open

Specification of Column units attribute values? #125

bmaranville opened this issue Feb 26, 2024 · 2 comments

Comments

@bmaranville
Copy link
Contributor

There is currently a test in orsopy.fileio.tests.test_schema that is expected to fail if the units for the first column are set to to the string: "test_fail_unit"

This is a problem, since in principle the Column schema needs to be applied to not just the first two columns ["Qz", "R"], but to any additional columns as well (after the first 4 columns, such as wavelength, incident_angle, etc.) which will not have the units specified manually in the current schema: {"enum": ["1/angstrom", "1/nm", "1", "1/s", None]}

We can address this in a couple ways:

  1. create a separate schema for additional columns, in which unit is any string
  2. change the Column schema so that unit is any string (which is the schema already defined in the Python code - the JSON schema is being manually modified to make it more restrictive in this spot)
@andyfaff
Copy link
Contributor

Is it possible to have the enumerated units for the first four columns, and any other string for additional columns, without creating a separate schema for those extra columns?

Will solution 2 permit any units for any column? I guess I don't mind this so much. If the default units aren't one of the enumerated ones I'd just go for the default of 1/angstrom.

Ultimately I prefer whatever solution is easiest to maintain.

@aglavic
Copy link
Collaborator

aglavic commented Feb 27, 2024

I support solution 2 with a custom code in _read_header_data that check this additional constraint.

Lookin at it I realized that there is actually no user function to trigger the schema validation. We should consider adding it to either the Orso class directly (for check in safe and load scenarios) or to load_orso as keyword argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants