-
Notifications
You must be signed in to change notification settings - Fork 4
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
Check datasets shape and dtype #16
Conversation
✅ Deploy Preview for ome-ngff-validator ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
b9153f8
to
a22aa14
Compare
Nice, @will-moore! I got some "Error loading" messages from the URLs above. Also: it might be worth considering how we maintain multiple validators for these non-json-schema constraints. Would defining "rule numbers" for each of them be feasible/useful? |
As expected! The "Error loading" is what you get if you use an invalid version, since we use the version to construct the schema URL. We don't try to maintain a list of all valid versions in this tool. I guess we could try to handle that 404 with a more custom message. Other errors for the other test URL are also expected (violations of rules 1, 2, 3 & 4. |
For me, "error loading" meant something like "the data is not accessible". 👍 for "incompatible version: ${version}", etc. |
@joshmoore: better? |
I like it! Thanks. One last thing: did the file for "4: dtype mismatch" get deleted? |
Hmmm - not sure why that got deleted. But here's another with the same dtype issue: |
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.
Beautiful. Merging.
This PR validates a few things that aren't checked by the schema validation:
/
when version is 0.2-0.4To test:
NB: This currently asserts that the dtypes are all equal for
v0.4
(so this PR can be tested). Need to revert to testingv0.5
and above BEFORE merging!For other tests above, we really need some invalid sample data available... Might try to make some...
cc @constantinpape