Improve messages related to parameter checks

[kilianvolmer]

Changes and Information

Please briefly list the changes (main added features, changed items, or corrected bugs) made:

• When parameters are compared to a tolerance, the message on failure prints the tolerance
• The formatting of values in messages is removed to reduce the possibility of wrong formatting
• For populations, we now return errors if values are smaller than -1e-10, otherwise warnings if the value is set to zero in apply_constraints().

If need be, add additional information and what the reviewer should look out for in particular:

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

• Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
• New code adheres to coding guidelines
• No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
• Tests are added for new functionality and a local test run was successful (with and without OpenMP)
• Appropriate documentation within the code (Doxygen) for new functionality has been added in the code
• Appropriate external documentation (ReadTheDocs) for new functionality has been added to the online documentation
• Proper attention to licenses, especially no new third-party software with conflicting license has been added
• (For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.

Checks by code reviewer(s)

• Corresponding issue(s) is/are linked and addressed
• Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
• Appropriate unit tests have been added, CI passes, code coverage and performance is acceptable (did not decrease)
• No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
• On merge, add 2-5 lines with the changes (main added features, changed items, or corrected bugs) to the merge-commit-message. This can be taken from the briefly-list-the-changes above (best case) or the separate commit messages (worst case).

Closes #1413 #1415 #1416

[codecov]

Codecov Report

❌ Patch coverage is 97.29730% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.28%. Comparing base (6233316) to head (f6c997a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cpp/memilio/epidemiology/populations.h	60.00%	2 Missing ⚠️
cpp/models/ode_mseirs4/parameters.h	95.45%	1 Missing ⚠️
cpp/models/ode_secirts/parameters.h	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
- Coverage   97.29%   97.28%   -0.02%     
==========================================
  Files         180      180              
  Lines       15646    15667      +21     
==========================================
+ Hits        15223    15241      +18     
- Misses        423      426       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mknaranja]

For populations, we now return errors if values are smaller than -1e-10, otherwise warnings if the value is set to zero in apply_constraints(). After some consideration, I am unsure if we want to blow up the code similarly for all models. I'd rather go with no. Maybe a slightly bigger refactoring with lambda functions such as in the RSV model might be more suited anyway and would pave the base for these additional changes.

[reneSchm]

I do agree with Martin, in that we should avoid adding more code duplication to the constraints methods, they are full of that as is.

Though I do not see the point of promoting a warning to an error with the exact same text. Maybe just set it to error every time? It would be more helpful to update the message itself, since it comes from apply constraints, not a check, and I think using both "value" and "index" would be more accurate than either "size" or "number". Also, in general, do not use hardcoded tolerances, or other magic numbers. In this case we already have Limits::zero_tolerance.

On the topic of removal of formatting: I did recommend doing so, because as far as I can tell, if formats are used, they more often outdated than (potentially?) useful. E.g., some integers are formatted as floats, or vice versa.

Printing tolerances seems good. However, we currently are loosing the context of "we think this is too close to 0" in the message.

[kilianvolmer]

Though I do not see the point of promoting a warning to an error with the exact same text. Maybe just set it to error every time?

That is basically the status quo. The idea behind the different types of messages was that error messages hint at something going actually wrong, while warnings just tell you to take a closer look, but it is probably fine. An example for the first one is a negative value for something that needs to be positive, while the latter is e.g. a floating point error that is set to 0.

[reneSchm]

The idea behind the different types of messages

You do not need to explain to me what log levels are, thank you.
The part you should be considering/addressing is: why adaptively change the level, when what you do and tell does not change at all?

[kilianvolmer]

The part you should be considering/addressing is: why adaptively change the level, when what you do and tell does not change at all?

I was trying to answer that: We do it for different reasons. Either because a parameter is actually wrong (error) and we fix it to be able to continue with the simulation or because we just have numerical issues (warning) and still fix it because otherwise the simulation (or functions using the simulation output) may run into unnecessary trouble.

[reneSchm]

But that differentiation does not matter - the "do and tell" part is important. We react to both cases in the exact same way (i.e. clamping to 0), and give the exact same log message. So you are just hoping that someone has the same associations with the log levels that you made.

As a general guideline, the log level is not part of the message you want to send, just a way to categorize (and filter by) importance.

Hence, the bare minimum would be change the message for each case. But as both cases have the same end-result, that seems unnecessary. Thus my proposal of always using error for higher visibility. The message already contains new and old value, so the user can already see how bad the correction was.

Maybe what you want to do is change how the apply_constraint result is handled in your code? I don't know, you have not given me much context to work with.