em_framework: Improve log messages, warning and errors #300
Conversation
EwoutH
changed the title
|
This looks very good and very useful. I'll try to take a look over the coming days. |
| @@ -346,14 +345,16 @@ def __init__(self, msis, n_processes=None, maxtasksperchild=None, **kwargs): | |||
| if isinstance(n_processes, int): | |||
| if n_processes > 0: | |||
| if max_processes < n_processes: | |||
| warnings.warn(f"the number of processes cannot be more then {max_processes}") | |||
There was a problem hiding this comment.
why did you drop userwarning here?
| # TODO: We should determine a version in which this method is removed and add that to the deprecation warning | ||
| warnings.warn( | ||
| "The 'retrieve_output' method is deprecated and will be removed in a future version. Use 'model.output' instead.", | ||
| DeprecationWarning, |
There was a problem hiding this comment.
This has been deprecated for several years. We might consider removing it.
There was a problem hiding this comment.
I added that it will be removed in version 3.0
| warnings.warn( | ||
| "HyperVolume is deprecated, use ArchiveLogger and HypervolumeMetric instead", | ||
| warnings.DeprecationWarning, | ||
| DeprecationWarning, |
There was a problem hiding this comment.
this was deprecated in the latest version, so it would make sense to remove it either in the next version or the one thereafer.
There was a problem hiding this comment.
I added that it will be removed in version 3.0
| raise EMAError(f"outcome {self.name} should be a collection") | ||
| raise EMAError( | ||
| f"Outcome {self.name} should be a collection, but is a {type(values)}: {values}" | ||
| ) | ||
| return values |
There was a problem hiding this comment.
this exception was recently triggered in some work of one of my PhD students. so this error is already better. Not sure about printing the full values because this could be something quite big.
Also, this might be better as a TypeError?
There was a problem hiding this comment.
What's the idea behind EMAError? Are there different types of EMAErrors? Would it be a user mistake, system failure, or can it be both?
There was a problem hiding this comment.
EMEError is a catchall error for stuff within the workbench going wrong. The main use case is if something goes wrong in the interaction between the workbench and the model we are connecting with. This can be a user mistake but also some system failure.
There is also the CaseError, which is exclusively used if a single experiment fails (e.g., a numerical error in a vensim model).
There was a problem hiding this comment.
Removed printing the full values and changed to TypeError.
|
Thanks for reviewing so quickly. I will work on you comments after #299 is finished. |
Improve the log messages, warning and errors, 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. There are two placed where it's mentioned that a function is deprecated, but not when they will be removed. I added TODO's there, I think we should decide on a version in which we want to remove them.
EwoutH
force-pushed
the
improve-warnings-and-errors
branch
from
42477c9
to
e0c0f9d
Compare
|
Processed the feedback. Added that both deprecated functions will be removed in 3.0. Squash and merge when ready. |
EwoutH
changed the title
EwoutH
changed the title
EwoutH
changed the title
Improve the log messages, warning and errors in the em_framework, by:
This will make debugging easier, allowing more time for model development and experimentation.
There are two placed where it's mentioned that a function is deprecated, but not when they will be removed. I added TODO's there, I think we should decide on a version in which we want to remove them.