Fix target data standardization and facet_nrow warning message

[LucieContamin]

• [x] Resolve #21

• [x] Resolve #19

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (9efc542) to head (16812ca).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #23 +/- ## =========================================== + Coverage 99.15% 100.00% +0.84% =========================================== Files 2 5 +3 Lines 473 499 +26 =========================================== + Hits 469 499 +30 + Misses 4 0 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[LucieContamin]

I merge the style PR #24 in this branch

[github-actions[bot]]

🚀 Deployed on https://661eaf9584318e49ae181e00--hubvis-pr-previews.netlify.app

[annakrystalli]

Docs Look nice eyh? 😉

[LucieContamin]

Docs Look nice eyh? 😉

It looks very nice! thanks again

[harryhoch]

So, I hate to be a stickler, but I'm not a fan of this..

"Before plotting, the data might require some preparation (filtering, etc.). These step is skipped in this example, please consult the “Plot Model Projections Output” vignette contained in this package for complete examples."...

Is there any reason not to include the code on this page?

[nickreich]

Noting that this might be a reason to fast-track adding standard data objects to the package. see #27

[nickreich]

I am going through my github notifications and see that my review has been requested here. Is this ready for review now, or still WIP?

[nickreich]

I guess one general comment I have is that if this is a breaking change (which it is, given that argument names are being renamed entirely without backwards compatibility), then I think we should maybe version increment a bit more aggressively? Maybe this could be 1.0?

[LucieContamin]

It is ready for Review. I am still thinking about the example question, but we might want to solve that question in another PR.

[LucieContamin]

I guess one general comment I have is that if this is a breaking change (which it is, given that argument names are being renamed entirely without backwards compatibility), then I think we should maybe version increment a bit more aggressively? Maybe this could be 1.0?

Thanks, that's a good point. It is a breaking change and so a "major" release. I was hesitant to release a first 1.0 version. I will update it

[LucieContamin]

Thank you for the review.

To answer your comments:

1. Will changing to 0.0.0.9100 be better to indicate the package is still in dev, but the last implementation include a major change?
2. The lintr check is about the complexity of the function as I understand. To solve it will require a major redesign of the code. I temporary solution will be to include an exception in the .lintr file of the package to temporary ignore that issue and maybe look into it in another PR?

[annakrystalli]

Re: Versioning, this is a topic I think we should discuss at a meeting for all our packages. (Sorry, it was a footer topic to discuss on my list of hubverse org changes but we decided to postpone and I didn't get to the bottom of the list!)

0.0.0.9*** indicates the package is still under development and version 0.0.1 would be the first stable release of a package after which semantic versioning applies more strictly.

I believe a number of our packages are at that stage, e.g. hubValidations and I'm planning to release hubUtils, hubAdmin and possibly hubData with 0.0.1 version when split seeing as the functionality is mature enough by now.

So I leave it up to @LucieContamin really to decide whether the functionality is stable enough for 0.0.1 or she still feels a 0.0.0.9* is still more appropriate. Incrementing by 10 instead of 1 might be more appropriate if so.

I suggest we also think about this with respect to hubEnsembles.

Hope this helps! Perhaps @bsweger has some more useful insights from industry too.

[annakrystalli]

Sorry @LucieContamin ! You basically said exactly the same but much more concisely 😆.

[LucieContamin]

The PR is becoming quite complex so please find here a small summary of the content:

• Fixes #25 [already reviewed]
• PR #29 [already reviewed]
• Fixes #19
• to fix lintr issues:
  • update code style
  • simplify function plot_step_ahead_output() and internal function plot_output() by creating multiple small functions
    • palette_utils: contains the internal code for the plot palette
    • scatter_plot_utils: contains the internal functions to create a plot (preparation of the data, add trace on a plot, etc.)
    • subplot_utils: contains the internal functions to create the output plot (create simple or complex subplot, etc.)
    • validate_format: contains the internal functions to validate the input parameters of plot_step_ahead_output()
• update the target data related parameters:
  • fixes #21
  • change the default parameters x_target_col_name = "date"
  • expect the target data input to have a column "observation" (previously "value")
• use the data from hubExamples in the documentation
  • remove inst files (previously used for examples)
• update the tests and documentation accordingly

I will prefer to wait before moving to version 0.1. I would like to solve 2 main issues before:

• finish solving #16 (currently fix only work on static plot)

• 13

[LucieContamin]

@annakrystalli thank you very much for your review!

So, I hope I fix the output_type_validation() issue. I also accepted your suggestion, thank you for that!

In another PR, we notice that the function was having an issue with empty columns in the model_output_data parameter, so I just added a small check and warning output in the plot_prep_data() function (in the scatter_plot_utils.R script). I updated the News accordingly.

Thanks for the test suggestion, it's on the issue #13 . I plan to update it before moving to version 0.1.

Please let me know if any other issues or questions! Thanks.

[annakrystalli]

Great! I'm happy for you to merge. 🎉🎉🎉🚀