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

Simulation with negative start time does not work #411

Closed
matthiaskoenig opened this issue Sep 18, 2017 · 22 comments
Closed

Simulation with negative start time does not work #411

matthiaskoenig opened this issue Sep 18, 2017 · 22 comments

Comments

@matthiaskoenig
Copy link
Collaborator

It is valid to run simulations from a negative start time (start is only how the value is called which corresponds to the the start of the simulation). I.e. if I want a plot from -20 to 0 this is completely valid, but does not work in roadunner:

r.simulate(start=-20, end=0, steps=10)

~/envs/tellurium/lib/python3.5/site-packages/roadrunner/roadrunner.py in simulate(self, start, end, points, selections, steps)
   3577             o.steps = steps
   3578 
-> 3579         result = self._simulate(o)
   3580 
   3581         o.steps = originalSteps

~/envs/tellurium/lib/python3.5/site-packages/roadrunner/roadrunner.py in _simulate(self, opt)
   3330 
   3331     def _simulate(self, opt):
-> 3332         return _roadrunner.RoadRunner__simulate(self, opt)
   3333 
   3334     def getValue(self, *args):

RuntimeError: duration, startTime and steps must be non-negative

The only requirement is that start <= end, but not non-negativity.

This is needed for SED-ML simulations which use negative start times in combination with offsets.

@luciansmith
Copy link

luciansmith commented Sep 18, 2017 via email

@hsauro
Copy link

hsauro commented Sep 18, 2017 via email

@matthiaskoenig
Copy link
Collaborator Author

Yes, use case is here:
https://jjj.bio.vu.nl/models/experiments/martins2016_fig4b/

<listOfSimulations>
    <uniformTimeCourse id="sim0_model0_sigcpsba" initialTime="-20" outputStartTime="0" outputEndTime="120" numberOfPoints="1000">
      <algorithm kisaoID="KISAO:0000019"/>
    </uniformTimeCourse>
  </listOfSimulations>

There should be a initial integration duration of 20 which should not be depicted in the figure (the initial phase to oscillations), but the start time of the plot should be at 0.

This also often happens in experimental data where the pre-intervention phase is negative (for instance -100 to 0, than intervention is given from 0 to 10. The simulation would start at negative times and the trigger for an intervention event would be triggered at 0.

@matthiaskoenig
Copy link
Collaborator Author

And specification looks good. It does not exclude negative start times.

2.4.3.3.1 initialTime
The attribute initialTime of type double represents the time from which to start the simulation. Usually this will be 0

2.4.3.3.2 outputStartTime
Sometimes a researcher is not interested in simulation results at the start of the simulation (i.e. the initial time). To accommodate this in SED-ML the uniformTimeCourse class uses the attribute outputStartTime of type double. To be valid the outputStartTime cannot be before initialTime. For an example, see Listing 2.44.

@luciansmith
Copy link

This is really problematic, because 'time' has a specific meaning in an SBML model, and by definition in SBML, starts at 0 at the start of the simulation. If the model uses csymbol 'time' in it, it will mean 'the variable that starts at 0 at the beginning of the simulation'. CellML could handle this, since in that language, 'time' is a variable just like any other, and if you want to set its initial value to -20, you just set it. But there's no way to set the csymbol 'time' in SBML.

You could fake it well enough if you ran the simulation as if -20 was 0, and then shifted the results back 20. You could do this entirely on the Tellurium side: it would take the input 'run from -20 to 50', tell roadrunner 'run from 0 to 70', then subtract 20 from the 'time' column of data. This would work for all models that don't use csymbol 'time' in them. But it would fail (or at least behave differently from CellML) for any models that did. Maybe have Tellurium issue a warning for all SED-ML that requests a negative start time if the model contains csymbol 'time'?

@matthiaskoenig
Copy link
Collaborator Author

matthiaskoenig commented Sep 19, 2017 via email

@kirichoi
Copy link

If that's the case, the fix should be pretty simple. We can simply store the initial delta T, simulate from 0 nonetheless, and add delta T at the end to time vector.

@luciansmith
Copy link

I still worry that both the -20 and the +20 solutions are not what was intended by SED-ML, but I suppose as early adopters of the spec, we have a say in what the spec means--if we're setting up the SED-ML test suite by default, everyone else has to do it our way ;-)

@luciansmith
Copy link

As a followup: discussed this with the editors, and this solution is indeed the correct one: whatever state the model is in is assumed to be the 'initial state', and if 'time' is supposed to be -20 or +20 or whatever, it must itself deal with the fact that the csymbol 'time' in SBML means 'time from the simulation initial state', and add a delta itself. (i.e. it's a modeling issue, not a software issue.)

@kirichoi
Copy link

Good to here that. I will implement this solution asap.

kirichoi pushed a commit to kirichoi/roadrunner that referenced this issue Sep 21, 2017
kirichoi pushed a commit to kirichoi/roadrunner that referenced this issue Sep 21, 2017
@0u812 0u812 closed this as completed in f842bfb Sep 30, 2017
0u812 added a commit that referenced this issue Sep 30, 2017
@matthiaskoenig
Copy link
Collaborator Author

thanks

0u812 added a commit that referenced this issue Oct 11, 2017
@0u812
Copy link
Member

0u812 commented Oct 11, 2017

As Lucian pointed out, this causes problems with events etc. (#425).

@kirichoi kirichoi reopened this Oct 11, 2017
@luciansmith
Copy link

I wonder if we could solve this by splitting 'time' into two columns: something like 'internal time' and 'simulation time'. You would treat 'internal time' by leaving it as-is, and it would start with zero and end wherever. Then 'simulation time' would be offset by the required SED-ML shift. (The shift is not supposed to affect the internals of the model at all, including events, etc. But it will look weird if you know your event trigger is 'time>5' and it fires at time="-3".

@kirichoi
Copy link

This is indeed problematic when dealing with events. Not only that, it's already a problem when starting simulation from time points other than 0. E.g. when you run:

r = te.loada("""
model mymodel()
  var species X = 10
  at time>=5: X = 5
end
""")

result = r.simulate(3, 10)
r.plot()

image
I am not sure whether it's feasible, but one way to solve this issue might be adding
delta T = 0 - starttime
to all time values specified in events and apply the previously proposed solution.

@hsauro
Copy link

hsauro commented Oct 12, 2017 via email

@kirichoi
Copy link

Reading SBML spec, it seems like time is actually treated as an absolute value. That means not only our proposed solution is wrong, but also the way we handle simulation starting at t >0 is wrong.
Simulation always starts at t = 0, event always triggers at specified time point (t > 0)
If one start simulation at t > 0, simulation still starts from t = 0 but the output must be truncated.
If one start simulation at t < 0, neither reactions nor events will start. Only specific rules (AssignmentRule, AlgebraicRule, etc.) may take effect.

@luciansmith
Copy link

So, this is a confusing situation and I've tried to update the latest SED-ML spec to be more clear, but the upshot is:

You are supposed to run your model as-is, without changing the internal 'time' at all. The assumption is that if a SED-ML file tells you to that the 'initialTime' is -20, that means that the model has already been scaled so that when the internal time is zero, the reported time should be -20.

The upshot for tellurium is that SED-ML's initialTime is a post-processing step: it should be applied to roadrunner's output, not any input. You truncate your output if (and only if) the 'outputStartTime' is different from the 'initialTime'.

@luciansmith
Copy link

OK, coming back to this after four years...

From my perspective now, I think that the SED-ML decision was the wrong one: when simulating an SBML model, time starts at 0. It always has, and there's no way around it, since it's an implied variable. The call:

simulate(5, 10, 50)

doesn't request that simulation start at 0, but that output collection begin at 5 and go to 10. As such, a negative number as the 'start' argument cannot make sense in a roadrunner context.

If SED-ML wants to declare that non-zero 'initialTime' attributes mean 'the pre-initialized state of the model as loaded' that would be a tellurium issue with interpreting SED-ML, but not anything I would want to implement as part of the 'simulate' function.

For Matthias: if you want to declare that time zero is actually time -20, I would say that this is a post-processing step you could perform the data that roadrunner outputs. But since 'time' is an internal variable in an SBML model, there's no way to make roadrunner do what you want.

@matthiaskoenig
Copy link
Collaborator Author

Okay, this makes sense. But this should then be a validation rule/warning on SED-ML. I.e. if the model is an SBML model and simulation times are negative this is invalid.

@luciansmith luciansmith reopened this Sep 23, 2021
@luciansmith
Copy link

I looked at this yet again due to Herbert saying something, and I was wrong--our simulation start time isn't a data collecction start, but an actual 'we assume the model is set to this time' start. This in turn completely changed my perspective on what 't0' meant in an SBML model, and it made data collection with t0 < 0 actually much easier!

It turned out to be difficult to get pre-time==0 events to fire correctly, but I have them working in general--still need to test more, but it's looking good: #854

@luciansmith
Copy link

This is now done! The only thing that doesn't work is #857 which isn't a negative-start-time specific problem. So I'm claiming this is done! It'll be in the next release.

@luciansmith
Copy link

Also, if you have opinions on this matter, other perspectives would be helpful at sbmlteam/sbml-specifications#2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants