Skip to content

Shiny app forecaster options cleanup #63

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

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

nmdefries
Copy link
Contributor

Forecaster options must be unique or shiny fails in weird ways. Also check that to-be-plotted forecasters are empty even if not length-0. Sometimes we end up with a non-informative list c("") that doesn't trigger the empty df to be returned.

@nmdefries nmdefries requested a review from dsweber2 November 15, 2023 18:03
@nmdefries
Copy link
Contributor Author

nmdefries commented Nov 15, 2023

@dsweber2 Does this fix your plotting problem? I wasn't able to reproduce, but I was having another issue with the shiny app due to ensemble names no longer being unique. Your issue seemed similar -- 4 names being provided seems like it would be related to us generating the 4 aheads separately.

Testing shiny apps is hard in general. There are some snapshot-based tests to check that behavior is stable. We're probably not there yet, but unit tests and assertions/checks in-code would be useful. The app doesn't use so many functions, so we'd need to refactor it to isolate testable code.

Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally forgot you could branch branches, I'll have to remember that in the future.

That fixed it! Thanks for the help

@dsweber2 dsweber2 merged commit 0bd0b55 into ensemble Nov 15, 2023
@nmdefries nmdefries deleted the ndefries/unique-forecaster-options branch November 15, 2023 19:07
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