Merged
Conversation
This adds a check on the MLD in checkFieldsFast, which throws an std::runtime_error if MLD is outside [0, 10 km]. The lower bound is quite strict (could also be a few mm) and the upper bound is just there because the code requires an upper bound. In reality we just want to check the lower bound.
The check on MLD doesn't really belong in checkFieldsFast, because it's not a prognostic field. We're just checking if we get crazy input, not if neXtSIM is doing something wrong. This is better done with a simple assert in the thermodynamics routine - as I've done in this commit. This way is also better, because it's more like to be caught right away, before NaNs spread all over the solution.
docguibou
approved these changes
Jan 3, 2024
Contributor
docguibou
left a comment
docguibou
left a comment
There was a problem hiding this comment.
Worked ok for me, error message was clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a very minimal check to address issue #648. This solution has been tested and it works well.