Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENHANCEMENT] more informative error when no setting is provided #537

Closed
mbakker7 opened this issue Feb 8, 2023 · 4 comments · Fixed by #599
Closed

[ENHANCEMENT] more informative error when no setting is provided #537

mbakker7 opened this issue Feb 8, 2023 · 4 comments · Fixed by #599
Assignees
Labels
enhancement Indicates improvement of existing features priority 1 normal, deal with in the foreseeable future
Milestone

Comments

@mbakker7
Copy link
Collaborator

mbakker7 commented Feb 8, 2023

Pastas can not do certain tasks if no settings are provided in the StressModel. For example, when the series is not long enough for the provided warmup, I get the following error:
ValueError: Simulation contains Nan-values. Check stresses time series settings!.

This error is not informative. The series is not long enough and needs to be extended in the past. I suggest to state that no settings are provided to the StressModel so that pastas doesn't know what to do. With instructions on how to do that. Also, there may be easier ways to specify the settings.

@mbakker7 mbakker7 added the enhancement Indicates improvement of existing features label Feb 8, 2023
@martinvonk martinvonk added priority 0 high, deal with as soon as possible priority 1 normal, deal with in the foreseeable future and removed priority 0 high, deal with as soon as possible labels Mar 7, 2023
@mbakker7 mbakker7 added this to the 1.1 milestone Mar 21, 2023
@martinvonk
Copy link
Collaborator

Maybe make providing StressModel settings mandatory? Even though an input time series is long enough.

@martinvonk
Copy link
Collaborator

fill_before and fill_after function should not fill nans when settings are not provided. Raise the error there.

@dbrakenhoff dbrakenhoff modified the milestones: 1.1, 1.2 May 12, 2023
dbrakenhoff added a commit that referenced this issue May 12, 2023
- improve simulation has NaNs msg to include example code for stressmodel settings
- add traps for method is None in fill_before and fill_after, raise ValueError
- avoid issues with trailing or leading NaNs in time series by setting tmin/tmax to first/last valid index
- add more informative error message when weights and observations are not same length
- improve msg formatting
@dbrakenhoff dbrakenhoff mentioned this issue May 12, 2023
6 tasks
@dbrakenhoff dbrakenhoff linked a pull request May 12, 2023 that will close this issue
6 tasks
dbrakenhoff added a commit that referenced this issue Jul 10, 2023
@martinvonk
Copy link
Collaborator

Related to the time series settings.

I found out about there exists something like the TypedDict in Python. This can help us to be more informative about the settings have to be provided. For stresses the the following settings have to be known: 'sample_up', 'sample_down', 'fill_nan', 'fill_before', 'fill_after'. With a TypedDict users get syntax highlighting if these settings are not provided. Small change but maybe very useful.

So code would look something like this:

from typing import TypedDict

class StressSettings(TypedDict):
   sample_up: str
   sample_down: str
   fill_nan: str | float
   fill_before: str | float
   fill_after: str | float

@dbrakenhoff
Copy link
Member

Closed by #639

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates improvement of existing features priority 1 normal, deal with in the foreseeable future
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants