opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
778 stars 311 forks source link

StateTrajectory should enforce strictly increasing time #820

Open aseth1 opened 8 years ago

aseth1 commented 8 years ago

I believe that having stationary time (remaining the same over multiple rows) is an error case. It is physically impossible. If this was simply a collection of states then order in time would not matter (say from a monte carlo process) but trajectory to me means evolving through time and a model can not be in two poses at the same time.

This is related to PR #730 but can be resolved post Hackathon.

tkuchida commented 8 years ago

If this change is implemented, it will also be necessary to prohibit integrators from reporting states before and after an (instantaneous) event is processed. I don't think such a requirement currently exists.

chrisdembia commented 8 years ago

@sherm1 has mentioned that even events have slightly different times before and after. If events do generate two states with the same time, we can allow a max of two consecutive states at a single time.

chrisdembia commented 8 years ago

Or we can have some type of flag that controls whether a trajectory is strictly increasing or not.

tkuchida commented 8 years ago

In Simbody, PeriodicEventHandler::setEventInterval() requires eventInterval > 0.0, so perhaps it's safe to enforce strictly increasing times. The only other potential issue I can think of is that, for some CMC simulations in OpenSim 3.2, the last two rows of the states file are identical---but that can (and should) be fixed if it's still an issue on master.

chrisdembia commented 8 years ago

Thanks for looking into Simbody, and good point about the 3.2 bug. Hmm...

sherm1 commented 8 years ago

setEventInterval() is the spacing between events. It is reasonable for each event to produce a before and after state at the same time, although I don't think Simbody does this at the moment. There is currently no promise that we won't do that -- and logically I think that would be the correct behavior.

aseth1 commented 8 years ago

The majority of cases for not having purely strictly increasing time will not be integrator events it will be human error. People are going to stuff states into a StatesTrajectory and forget to update time or update it incorrectly causing duplicate times. This is the case I am more worried about.

chrisdembia commented 8 years ago

I think you're right. We'll move to strictly increasing time and make changes if necessary in the future.

aseth1 commented 8 years ago

I think we can all agree that physically a system cannot be in two states at the same time. Even in rigid impacts (that approximate reality) you are talking about an infinitesimal time before and after an event (or the event itself occurs over an infinitesimal period) over which the state changes - that is to say that the pre and post states are not at identical times. Isn't the issue of having identical times a consequence of how you represent an infinitesimal interval as 0? But you could equally use SimTK::Eps or does the theory require 0?

tkuchida commented 8 years ago

does the theory require 0?

PLUS impact theory is built on several assumptions, one of which is that the impact occurs in zero time (e.g., time-dependent forces are absent). Nudging time forward is philosophically incorrect. If the concern is that a user will accidentally store the same state twice, can the method check to ensure the new state is distinct from the previous one? If performance is an issue, perhaps the check can be done in debug mode only, or perhaps a unique ID in the state (if there is one) could be checked (clearly, time isn't the unique ID you're looking for :wave:). I don't think the modeling API should affect the physics of the problem.

chrisdembia commented 8 years ago

This is not going to be done before the hackathon. Perhaps let's discuss in a dev meeting after the hackathon?

sherm1 commented 8 years ago

But you could equally use SimTK::Eps or does the theory require 0?

I agree with Tom -- theory says 0, Eps would be a hack (though not entirely unreasonable). Even if we did it though, higher level software like StateTrajectory should not be depending on it. It might be nice to figure out how to be very explicit about it, so that the state pair [(t,before),(t,after)] at the same time t would be accompanied by a flag saying that an event occurred at t. Then higher-level code could disallow duplicate times except when an event is known to have occurred.

chrisdembia commented 8 years ago

Sounds sufficient for the State object to have a flag denoting if it is associated with an event (before or after).

sherm1 commented 8 years ago

For a great introduction to the theory of hybrid simulation, including a clear and rigorous discussion of the multiple-values-at-the-same-time issue, see Lee & Zheng 2005 in this chapter. Read the time discussion in sections 4 & 5, pp 33-35; you can skip the preliminaries. Here is an image from there: image

chrisdembia commented 8 years ago

Here's what I found to be relevant:

Given only the discrete samples, therefore, the discontinuous signal is fundamen- tally indistinguishable from a continuous signal that simply changes sufficiently rapidly. ... Having two values at one time is semantically unambiguously distinct from having two distinct values closely spaced in time.

chrisdembia commented 8 years ago

Thanks for sharing that.

jenhicks commented 8 years ago

@chrisdembia Are you planning to address this for 4.0 or hold for a future release?

chrisdembia commented 8 years ago

I'm not sure. It is not a priority right now.