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

Added warning and automatic fix for no bounds #166

Merged
merged 4 commits into from
Feb 14, 2020
Merged

Conversation

jvegreg
Copy link
Contributor

@jvegreg jvegreg commented Jul 15, 2019

As discussed in #151

@jvegreg jvegreg added enhancement New feature or request cmor Related to the CMOR standard labels Jul 15, 2019
@jvegreg jvegreg requested a review from bouweandela August 8, 2019 15:37
@bouweandela
Copy link
Member

Can you please fix the merge conflict?

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me, but I did not test

@jvegreg
Copy link
Contributor Author

jvegreg commented Aug 23, 2019

Done!

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine to me, but I did not try running it. @mattiarighi Could you please test?

@mattiarighi
Copy link
Contributor

I would suggest @nperezzanon to test it, since she reported the issue in #151.

@mattiarighi mattiarighi requested review from nperezzanon and removed request for mattiarighi September 13, 2019 15:04
@mattiarighi
Copy link
Contributor

@nperezzanon can you please test this so it can be merged?

@bouweandela
Copy link
Member

@mattiarighi Can you have a look at this please? Or maybe @axel-lauer can test, because this is a feature he requested?

@mattiarighi mattiarighi requested review from axel-lauer and removed request for nperezzanon December 19, 2019 12:29
@mattiarighi
Copy link
Contributor

@axel-lauer any use case for testing this?

@zklaus
Copy link

zklaus commented Feb 17, 2020

Just got aware of this via @jhardenberg's mention in #470. Every time this is actually used it masks a serious data error. What's more, guess_bounds is unreliable and will lead to wrong bounds for example for staggered grids.

@jvegreg
Copy link
Contributor Author

jvegreg commented Feb 17, 2020

Every time this is actually used it masks a serious data error. What's more, guess_bounds is unreliable and will lead to wrong bounds for example for staggered grids.

I agree with you that this can lead to errors, but:

  • In regular grids, guess_bounds is usually a sound approximation and it does not work in any other case

  • We are adding a warning in any case, so users should be aware that something may not be totally correct

  • Don't think too much about our computations in staggered grids: i.e we are using the same areacello for our computations on thetao, uo, and vo and we know that it should not be the same, at least for EC-Earth and other NEMO-based models

This is the kind of issue in which the trade-off strictness - usability we are discussing every time shows off in all its glory

@zklaus
Copy link

zklaus commented Feb 17, 2020

  • guess_bounds guesses that the points are in the middle of the cells. If the points are on the faces, edges, or corners of cells, as they can be in staggered, regular grids, it effectively shifts and/or distorts the cells.
  • Nobody reads the warnings because you get an impenetrable wall of text
  • I am not sure how that relates: In staggered grids the cells are the same also for points that live on faces, edges, and corners, consequently the cell areas are the same. Regardless, are you saying somebody is making an error without caring about it so nobody should care about it?

Regarding usability vs compliance, the case doesn't seem so clear. I absolutely think this can be fixed, so high usability from a user point of view, but imho really only on a case by case basis.

@bouweandela
Copy link
Member

Maybe we can revisit this after #374 is merged, because then we could only apply the guess bounds when using reduced cmor check strictness?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmor Related to the CMOR standard enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants