analysis: Improve log messages, warning and errors #313
Conversation
This file contains 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
Improve the errors in the ema_workbench/analysis, by: - Making them more explicit - Including what the variable value or type not expected currently is - Including what the variable value or type not expected should be - Stating the explicit type of warning or error (DeprecationWarning, ValueError, etc.) - Converting them to F-strings (for better in-code readability) - Formatting warnings and errors with a Capital letter, while leaving log-statements lowercase. This will make debugging easier, allowing more time for model development and experimentation.
for more information, see https://pre-commit.ci
quaquel
approved these changes
Nov 17, 2023
Apparently an empty list [] is allowed and provided in tests. Is this intended?
EwoutH
commented
Nov 17, 2023
| f"outcomes_to_show must be a list of strings or None, instead of a {type(outcomes_to_show)}" | ||
| ) | ||
| # TODO: Apparently an empty list [] is allowed. Is this intended? | ||
| elif len(outcomes_to_show) == 1: |
There was a problem hiding this comment.
@quaquel when I defined len(outcomes_to_show) < 2, test failed because the test where inputting an empty list [] (with length 0). Apparently this is allowed, is this intended?
There was a problem hiding this comment.
if you insert None, or an empty list, it defaults to all outcomes in prepare_data. See prepare_data.
There was a problem hiding this comment.
Right, then we should explicitly allow empty lists.
There was a problem hiding this comment.
Not sure. I know that a while ago, I changed everything explicitly to None instead of using empty lists. Can't recall exactly why (perhaps because lists are mutable?)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Improve the errors, warnings and log messages in the ema_workbench/analysis, by:
This will make debugging easier, allowing more time for model development and experimentation.
Very similar to #300, but then on the
analysispart instead of theem_framework.