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

1230 time offset #1232

Merged
merged 7 commits into from
Jun 28, 2024
Merged

Conversation

pchelle
Copy link
Collaborator

@pchelle pchelle commented Jun 19, 2024

No description provided.

@pchelle pchelle marked this pull request as ready for review June 19, 2024 12:32
Copy link
Member

Choose a reason for hiding this comment

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

Another question: is it guaranteed that the time point of the offset is included in the list of the output time points?
E.g. if my initial output raster is 0h, 1h, 2h, ... and my time offset is 30 min: will 30min be included in the output time points?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently do not update the output schema. So, the output time points won't necessarily include the offset

@pchelle pchelle requested review from KatrinCoboeken and Yuri05 June 26, 2024 10:50
@Yuri05
Copy link
Member

Yuri05 commented Jun 26, 2024

@pchelle Btw, it is indeed possible to add single time points to the simulation output schema via https://www.open-systems-pharmacology.org/OSPSuite-R/dev/reference/OutputSchema.html#method-addtimepoints-
However it's not quite clear for me, if the added values are automatically assumed as the internal time unit ([min]) or units can be passed additionally (just created issues for this in ospsuite-r)

Comment on lines 74 to 79
endTime <- ospsuite::toUnit(
quantityOrDimension = "Time",
values = simulation$outputSchema$endTime,
targetUnit = timeUnit
)
validateVectorRange(timeOffset, type = "numeric", valueRange = c(0, endTime))
Copy link
Member

Choose a reason for hiding this comment

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

Question: if the minimumSimulationEndTime is passed: is the simulation end time adjusted before or after this check? Because if it's done after the range bound should be max(simulation$outputSchema$endTime, minimumSimulationEndTime)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oups, before.
Also, I noticed I forced minimumSimulationEndTime to be within 0 and outputSchema$endTime...
What would be the expected range for this argument actually ? >0, [0, endTime], or [endTime, Inf]

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be >0
Also if minimumSimulationEndTime <= endTime I actually would not change the simulation end time.

Copy link
Member

Choose a reason for hiding this comment

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

Question not related to the current PR: in the lines 132 and 136, minimumSimulationEndTime is assumed to be in minutes

if (simulationSet$minimumSimulationEndTime > simulation$outputSchema$endTime) {
maximalIntervalIndex <- which(sapply(simulation$outputSchema$intervals, function(x) {
x$endTime$value
}) == simulation$outputSchema$endTime)[1]
simulation$outputSchema$intervals[[maximalIntervalIndex]]$endTime$setValue(value = simulationSet$minimumSimulationEndTime, unit = ospUnits$Time$min)

However when passed to the simulation set constructor, minimumSimulationEndTime is assumed to be in the user defined unit timeUnit (which is also [h] as per default).

SimulationSet <- R6::R6Class(
"SimulationSet",
public = list(
simulationSetName = NULL,
simulationFile = NULL,
outputs = NULL,
dataSource = NULL,
dataSelection = NULL,
timeUnit = NULL,
applicationRanges = NULL,
minimumSimulationEndTime = NULL,
timeOffset = NULL,
massBalanceSettings = NULL,

As long as there is no unit conversion when setting minimumSimulationEndTime it seems to be a bug?
@pchelle

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. minimumSimulationEndTime should be defined by user in timeUnit, and be converted in baseUnit in this function.
I created issue #1238 to fix this

@Yuri05 Yuri05 merged commit e8a9cca into Open-Systems-Pharmacology:develop Jun 28, 2024
3 of 4 checks passed
@pchelle pchelle deleted the 1230_time_offset branch August 9, 2024 08:33
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.

3 participants