Skip to content

Conversation

@vincenzocoia
Copy link

This pull request comes with three changes:

  1. A modified version of ch_regime_plot() (ch_regime_plot2()) so as not to depend on a specific data frame format as an input.
    • For example, can't easily use the original function if data are retrieved from tidyhydat.
  2. A function I like using called ch_ams_timeseries() to visualize a time series of annual maxima. Especially useful for making implicit NA's explicit, so that the line plot breaks when data for a year is missing.
  3. A scale_*_gumbel*() family of functions meant to be added to a ggplot object to put return period or AEP on reduced Gumbel variate spacing.

Regarding the regime plot, this is meant to generate a discussion regarding what to do about this new way of engaging with the function, especially since other plotting functions have the same problem.

@KevinShook
Copy link
Contributor

KevinShook commented Oct 31, 2025

Hi Vincenzo
Sorry that it's taken me so long to look at your pull request.
A few comments:

  • the conflicts noted above
  • errors in the check() log file
  • is it necessary to have the function ch_regime_plot2(), when we have the function ch_tidyhydate_ECDE?
  • it would be nice to have the function ch_ams_timeseries have a slightly more meaningful name indicating that it is a plotting function, and to have the example use an actual time series

@KevinShook
Copy link
Contributor

00check.log
Here's the check() log. Do you have an older version of ch_catchment_hyps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants