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

Notes on implementation; scoping compared to draft conformance rules and ugrid-checks #17

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pp-mo
Copy link

@pp-mo pp-mo commented Apr 21, 2022

Comments in code (no changes)

Scope

I believe there are only a few checks which cc-plugin-ugrid conducts which ugrid-checks does not.
-- And, as it happens, I think I disagreed with all those cases personally. E.G. checking numbers of coordinates against topology dimension, which I think is too narrow as I already explained in this comment in a ugrid conventions discussion

Summary of Problems noted

  • both here and here, subsidiary test routines are called, but in both cases, if they fail, they make no stateful change but only return a Result object -- which the caller discards instead of incorporating into its own Result object.
    I assume the intention here was to add them to a subsidiary Result list.
  • this message and this one need to be generalised to refer to either faces or nodes
  • this lines seem to have the possibility of crashing the checker code, if there is an attribute referring to a non-existent variable (but N.B. I haven't checked this yet)
    • and the following line might also prove unsafe, if a referred connectivity variable doesn't have exactly 2 dimensions ?
  • this check seems to rule out the use of quadrilateral faces (or flexible meshes) altogether ??
  • code here and here I don't understand at all. It seems to hardwire dimension names, which is surely wrong ??
    This also crashes, instead of producing an error result (which tends to get ignored)

@pp-mo pp-mo marked this pull request as draft April 21, 2022 11:29
@pp-mo pp-mo changed the title Notes on implementation; scoping compared to draft conformance rules … Notes on implementation; scoping compared to draft conformance rules and ugrid-checks Apr 21, 2022
@ocefpaf
Copy link
Member

ocefpaf commented Oct 29, 2024

@pp-mo I'd like to deprecate this one in lieu for ugrid-checks, but I'd like to see if we can make ugrid-checks work as a compliance-checker plugin first. Probably not an easy feat...

@pp-mo
Copy link
Author

pp-mo commented Nov 12, 2024

@ocefpaf see if we can make ugrid-checks work as a compliance-checker plugin

Thanks for the plug !

I did look into wrapping ugrid-checks within the ioos framework before, but only as a preliminary investigation.
It was discussed in ioos/ioos-code-sprint#6
My own ideas notes here : pp-mo/ugrid-checks#44
Meanwhile a draft (partial) code interfacing approach is visible here.

I would note that, when I discussed with another person (? @MathewBiddle or someone else ?) in context of the existing code, it seemed that the existing implementation was a more structured validation approach than the ugrid-checks code, which is arguably a bit ad-hoc in it's approach (it was written under some time constraint). I think you can see that in the ioos-interfacing draft code outlined above .

I think it's also worth saying that, like the UGRID spec itself, the ugrid-checks code has been pretty much static for some years + gets very little attention, or at least little feedback.
Which is not to say that it doesn't provide some value, and is still functional.

So it does need careful consideration whether the somewhat hacky ugrid-checks code, and the above interfacing approach is generally good enough to adopt here -- I will appreciate comment on that from others !

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

Successfully merging this pull request may close these issues.

2 participants