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

Add observed data to time profiles #674

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

rengelke
Copy link
Contributor

Adds a new parameter observedData to the function sensitivityTimeProfiles to include observed data in the plots. Closes #259

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 99.02913% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.94%. Comparing base (830d772) to head (fbfb44a).

Files Patch % Lines
R/sensivitity-time-profiles.R 98.93% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   85.62%   85.94%   +0.32%     
==========================================
  Files          29       29              
  Lines        2163     2220      +57     
==========================================
+ Hits         1852     1908      +56     
- Misses        311      312       +1     

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

Comment on lines 453 to 454
timeSeriesDataFrame <- timeSeriesDataFrame %>%
dplyr::filter(grepl("Concentration", Dimension))

Choose a reason for hiding this comment

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

Wondering if restricting to Concentrations is really needed?

Copy link
Member

Choose a reason for hiding this comment

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

No, any kind of dimension should be supported.

@@ -21,6 +21,10 @@
#' (linear scale), to set the x-axis scale. Default is "lin".
#' @param yAxisScale Character string, either "log" or "lin", sets the y-axis
#' scale similarly to `xAxisScale`. Default is "log".
#' @param observedData Optional. A named set of `DataSet` objects containing
Copy link
Member

Choose a reason for hiding this comment

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

Why must the list be named?

@@ -158,13 +163,21 @@ sensitivityTimeProfiles <- function(sensitivityCalculation,
)
)

# add observed data to time series data frame
Copy link
Member

Choose a reason for hiding this comment

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

Add validation:

ospsuite.utils::validateIsOfType(observedData, DataSet, nullAllowed = TRUE)

Comment on lines 453 to 454
timeSeriesDataFrame <- timeSeriesDataFrame %>%
dplyr::filter(grepl("Concentration", Dimension))
Copy link
Member

Choose a reason for hiding this comment

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

No, any kind of dimension should be supported.

@@ -242,6 +271,22 @@ sensitivityTimeProfiles <- function(sensitivityCalculation,
na.rm = TRUE
)

# add line for observed data
if (addObeservedData) {
Copy link
Member

Choose a reason for hiding this comment

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

Observed data should be added as symbols, not as lines.

Copy link
Member

Choose a reason for hiding this comment

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

Why did the analysis results change?

Also the legend for parameter factor is incomplete now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a mistake in the previous test for multiple output paths where results instead of results_multiple (now resultsMultiple) is used for plotting:

test_that("sensitivityTimeProfiles plots are as expected for multiple output paths", { set.seed(123) p_list <- sensitivityTimeProfiles(results)

@@ -242,6 +272,22 @@ sensitivityTimeProfiles <- function(sensitivityCalculation,
na.rm = TRUE
)

# add line for observed data
Copy link
Member

Choose a reason for hiding this comment

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

"add symbols"?

@rengelke rengelke requested a review from PavelBal July 12, 2024 19:31
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.

sensitivityTimeProfiles: add observed data
3 participants