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

address #537 #599

Merged
merged 3 commits into from
Jul 10, 2023
Merged

address #537 #599

merged 3 commits into from
Jul 10, 2023

Conversation

dbrakenhoff
Copy link
Member

@dbrakenhoff dbrakenhoff commented May 12, 2023

Short Description

  • 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

I hope this improves feedback to the user without limiting any functionality. Let me know if I missed any cases...?

Examples

Case with stress starting after oseries. Running ml.solve() yields the following exception:

ValueError: Time Series 'rain': cannot be extended into past to 1975-11-17 00:00:00 as 'fill_before' 
method is 'None'. Provide settings to stress model, e.g. `ps.StressModel(stress, settings='prec')`.

Case with stress ending before oseries. Running ml.solve() yields the following exception:

ValueError: Time Series 'rain': cannot be extended into future to 2015-06-28 00:00:00 as 'fill_after' 
method is 'None'. Provide settings to stress model, e.g. `ps.StressModel(stress, settings='prec')`.

If somehow NaNs still make it into the simulation:

ValueError: Simulation contains NaN-values. Check if time series settings are provided for each stress 
model (e.g. `ps.StressModel(stress, settings='prec')`!

If somehow simulation and observations do not match:

ValueError: Weights and observations time series have different lengths! Check oseries and stresses settings.

Checklist before PR can be merged:

- 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 added the enhancement Indicates improvement of existing features label May 12, 2023
@dbrakenhoff dbrakenhoff added this to the 1.2 milestone May 12, 2023
@dbrakenhoff dbrakenhoff linked an issue May 12, 2023 that may be closed by this pull request
@codacy-production
Copy link

codacy-production bot commented May 12, 2023

Coverage summary from Codacy

Merging #599 (16c4dd3) into dev (a7b11cc) - See PR on Codacy

Coverage variation Diff coverage
-0.15% (target: +0.00%) 38.46%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (a7b11cc) 4955 3599 72.63%
Head commit (16c4dd3) 4965 (+10) 3599 (+0) 72.49% (-0.15%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#599) 13 5 38.46%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

dbrakenhoff and others added 2 commits May 12, 2023 17:48
Because the metrics can also be used directly without Pastas as well :-)
@dbrakenhoff dbrakenhoff merged commit 676d76b into dev Jul 10, 2023
9 of 10 checks passed
@dbrakenhoff dbrakenhoff deleted the 537-no-settings-error branch July 10, 2023 17:16
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] more informative error when no setting is provided
2 participants