Skip to content

Conversation

@VEZY
Copy link
Member

@VEZY VEZY commented Apr 30, 2025

Refactoring the code to make it easier to maintain. The old code was a nightmare; changing a simple thing had many side effects. Now we are trying to separate the plotting code of each kind of plot, so updating the code of a type of plot will be simply looking at the code for it.

This PR continues the previous one that I accidentally merged: #57

VEZY added 5 commits April 30, 2025 15:08
Remove duplicated code
Remove default plot title when all_situations = TRUE (the plot title is used to identify the situation, so no need if all_situations = TRUE)
@VEZY
Copy link
Member Author

VEZY commented Apr 30, 2025

Update:

All dynamic plots (figure 1 to 8) where visually checked and are valid.

We checked the scatter plots that are currently implemented (see below).

The plots with all_situations=FALSE will produce several plots, one per situatio. In the tests we currently consider that the plotting function only returns one plot. So we have to add that (maybe add a column in the CSV file of how many plots we expect?)

Here's the state for the scatter plots:

  • 1: checked, see Implement scatter plot per situation #69. There is a difference (the test fails), but it is only because now we have a square plot with the same boundaries for the x and y axes (which is a desired behavior).
  • 2: checked
  • 3: checked -> the reference from the release is wrong, it plots the residuals, not the variable values. This should be figure 24. I didn't test it again by myself, but it may be a bug in the release. Anyway, the new one looks good.
  • 4: same than 3 though, the reference mistakenly used the residuals. The new one looks good.
  • 5: checked. The letter "a" appeared in the legend as we had geom_text_repel showed up in the legend. I removed it in 661f527.
  • 6: checked
  • 7: checked. The old released version was wrong (but most probably from the code of the test, not the plot itself). Anyway now it is good.
  • 8: implemented in 887d990. Checked
  • 9: implemented in 887d990. Checked
  • 10: Implemented in 148db73. Need corrections: we need one regression line per version. Improvement suggested: the regression line should have the same color than the points (== version)
  • 11: Implemented. Same than 10.
  • 12: Implemented in 71a9753 Colour text per version.
  • 13: Implemented in c3d8a1a). Need corrections: we need one regression line per version => Done
  • 14: to implement. Need corrections: we need one regression line per version => Done
  • 15: checked -> problem with the color of the error bars (black instead of colored after the situation) => Done
  • 16: checked. See Wrong color when grouped USMs give only one situation #61 For improvements in a next release.
  • 17: implemented in d7bcccc. Modified in dd6d847. Need corrections: we need one regression line per version, colored after the version => Done
  • 18: implemented in 2222d85, not checked. Need corrections: same than 17 => Done
  • 19: implemented in 2222d85, not checked. Need corrections: same than 17 => Done
  • 20: implemented in 2222d85, not checked. Need corrections: same than 17 => Done
  • 21: implemented in 2222d85, not checked. Need corrections: same than 17 => Done
  • 22: implemented in 2222d85, not checked. Ylim not adapted.
  • 23: checked. We have residuals in our version, which is normal as we give them in the tests. Not providing them results in the same plot as the reference.
  • 24: checked.
  • 25: checked. The boundaries in y are better in this PR that in the release for masec.
  • 26: checked.
  • 27: checked. We add the error bars now (they are required in the CSV file)
  • 28: checked. Same than 27.
  • 29: checked.
  • 30: thanks to c43086d. Ylim not adapted.
  • 31: thanks to c43086d. Modified in 93d2c39. Need corrections: same than 17. Title of the test (see csv file) should have version.
  • 32. Implemented. Need corrections: same than 17 => Done. Ylim not adapted.
  • 33. Implemented. Need corrections: same than 17.
  • 34. Implemented in 71a9753. Colour text per version.
  • 35. Implemented. Need corrections: same than 17 => Done
  • 36. Implemented. Need corrections: same than 17 => Done
  • 37: checked. Color of the error bar should be the same as the points == situation (not black) => Done
  • 38: checked. Color of the error bar should be the same as the points == situation (not black). Need corrections: same than 17 => Done
  • 39: implemented in 2222d85. Need corrections: same than 17 => Done. Ylim not adapted.
  • 40: implemented in 2222d85. Need corrections: same than 17 => Done
  • 41: implemented in 2222d85. Need corrections: same than 17 => Done
  • 42: implemented in 2222d85. Need corrections: same than 17 => Done
  • 43: implemented in 2222d85. Need corrections: same than 17 => Done

@github-actions
Copy link

github-actions bot commented Jun 4, 2025

📝 Styler has automatically formatted the following R files in this PR:

  • tests/testthat/test-dynamic-plots.R

Please pull these changes to your local branch.

@github-actions
Copy link

github-actions bot commented Jun 4, 2025

📝 Styler has automatically formatted the following R files in this PR:

  • R/cropr_formatting.R
  • R/generic_plotting.R
  • R/specific_plotting_dynamic.R

Please pull these changes to your local branch.

@codecov
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.20%. Comparing base (13b9a50) to head (872cd1d).
Report is 196 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   61.44%   64.20%   +2.76%     
==========================================
  Files          17       20       +3     
  Lines        1250     1506     +256     
==========================================
+ Hits          768      967     +199     
- Misses        482      539      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

VEZY and others added 27 commits January 14, 2026 11:37
use numbering for all sections
re-work text a little bit
Fix issue with simulations in patchwork -> simulations were wrongly modified above
@github-actions
Copy link

📝 Styler has automatically formatted the following R files in this PR:

  • R/extract_plot.R

Please pull these changes to your local branch.

@sbuis sbuis merged commit c92364b into main Jan 23, 2026
@sbuis sbuis deleted the code-refactoring branch January 23, 2026 14:56
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.

5 participants