eagles-project / mam4xx

A C++ implementation of MAM4
https://eagles-project.github.io/mam4xx/
Other
6 stars 6 forks source link

Update CMakeLists.txt for Validation Diffs Due to Relative Error Change #331

Closed mjs271 closed 2 weeks ago

mjs271 commented 3 months ago

This PR is in response to the mam_x_validation PR #72 that changed validation testing to always use relative error as the pass/fail criteria.

Unfortunately, this led to some reduced tolerances and some failing tests due to NaN values. In response, I made edits to the relevant CMakeLists.txt files to result in all enabled tests passing. I have denoted all changes with FIXME comments that mention the change to relative error criteria. Most tests lost an order of magnitude or two in accuracy, which may not necessarily be a dire issue, though a few ended up with relative errors that were quite large, e.g., $\geq \mathcal{O}(1)$. However, the tests with hidden NaN values will likely require attention, and they are currently disabled.

Update: The NaN's were a result of my mistake--caused by zero-error that needed to be checked for and is fixed with the latest push.

jeff-cohere commented 3 months ago

Can we definitely state that we should be using relative errors to judge all test results? In my experience this stuff often requires some judgement based on the quantities themselves. I'm curious what the reviewers of thje related mam_x_validation PR think of this.

mjs271 commented 3 months ago

Can we definitely state that we should be using relative errors to judge all test results? In my experience this stuff often requires some judgement based on the quantities themselves. I'm curious what the reviewers of thje related mam_x_validation PR think of this.

There's definitely room for debate--this was just what made sense to me. I was surprised by how many tests have large relative errors, like $\mathcal{O}(1)$ and greater, but were still passing based on the previous criteria.

Upon thinking on it a little more, I suppose there could be cases where the relative error could be skewed by entries in an output array that vary many orders of magnitude (I think?). However, I don't yet have well-formed ideas on how to handle that case