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

Fixes #934 qualification workflows handle dual axis in time profiles #993

Merged

Conversation

pchelle
Copy link
Collaborator

@pchelle pchelle commented Feb 20, 2023

Also Fixes #985

The fix requires PR 407 from tlf package and might have side effects on the legend order of other plots performed by the reporting engine

@Yuri05
Copy link
Member

Yuri05 commented Feb 23, 2023

5 tests fail on windows...

@pchelle pchelle marked this pull request as ready for review February 24, 2023 11:11
Comment on lines 216 to 221
if (isOfLength(requestedAxes, 3)) {
logError(messages$warningTooManyAxes(
project = timeProfilePlan$Project,
simulation = timeProfilePlan$Simulation
))
requestedAxes <- c("Y", "Y2")
Copy link
Member

Choose a reason for hiding this comment

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

Why this check 2 times (here and above lines 189-191)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy/Paste typo.
I am going to remove it.

Comment on lines 229 to 230
simData$yAxis <- ifNotNull(simData$yAxis, simData$yAxis %in% tail(requestedAxes, 1))
obsData$yAxis <- ifNotNull(obsData$yAxis, obsData$yAxis %in% tail(requestedAxes, 1))
Copy link
Member

Choose a reason for hiding this comment

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

@pchelle Can you explain what happens in these 2 lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the right axis feature in the {tlf} package, the variable mapping which axis is used needs to provide logical values such as TRUE correspond to values on the right axis and FALSE values on the left axis.

For intellisense, I am replacing yAxis by y2Axis while adding the explanations below

  • FALSE y2Axis values correspond to data on the left axis
  • TRUE y2Axis values correspond to data on the right axis
  • Since yAxis values are included in "Y", "Y2" or "Y3", TRUE values will correspond to the values included (%in%) last found axis

@codecov-commenter
Copy link

Codecov Report

Base: 76.38% // Head: 76.30% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (aa47270) compared to base (0c53346).
Patch coverage: 79.74% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #993      +/-   ##
===========================================
- Coverage    76.38%   76.30%   -0.08%     
===========================================
  Files           64       64              
  Lines         8430     8563     +133     
===========================================
+ Hits          6439     6534      +95     
- Misses        1991     2029      +38     
Impacted Files Coverage Δ
R/messages.R 77.77% <ø> (ø)
R/utilities-ratio-comparison.R 82.85% <57.14%> (ø)
R/qualification-time-profile.R 55.20% <79.70%> (+6.15%) ⬆️
R/utilities-configuration-plan.R 90.90% <85.71%> (-0.52%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Yuri05 Yuri05 merged commit a396c41 into Open-Systems-Pharmacology:develop Feb 24, 2023
@pchelle pchelle deleted the 934_qualification_dual_axis branch February 15, 2024 16:26
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.

Qualification workflow / Time profile plots: add 2nd y-axis automatically if required
3 participants