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

Remove the option for using kinematics for ExternalLoads #2334

Closed aseth1 closed 3 years ago

aseth1 commented 5 years ago

What does the "Kinematics for external loads" option actually do and why should anyone care?

When specifying external loads the force(s) and point(s) of application are typically specified in the lab (ground) reference frame, but applied to points on the model (e.g. the foot). The option provides a preprocessing step that re-expresses the force and point (originally in ground) into the applied-to body (Frame) frame of reference. In the case that the motion being analyzed (e.g. InverseDynamicsTool, StaticOptimization and any other Analysis of the AnalyzeTool involving applied ExternalLoads) the answers will be identical IF the kinematics being analyzed and the kinematics for external loads (for transforming the force into the body) are identical. In the cases that kinematics are not identical (choosing wrong file, down sampling, filtering, etc...) the transformation introduces inconsistencies.

So why do it? The ONLY reason to register forces using input kinematics instead of just applying them in the lab frame is to "mitigate or reduce" drift of the RRA and particular CMC solutions which are tracking those kinematics. If the model kinematics are deviating from the input kinematics the conjecture is that large forces (e.g at the foot) applied in ground reference frame will no longer be on the model foot and it will hasten the divergence of the tracking solution. While the first part is true, if tracking errors are large the foot can be out of position and the applied force (in ground) are way off the foot. The question is whether registering the forces to the foot improves the answers, and that is definitely, NO! If the solution is tracking, the differences are insignificant. If CMC is diverging, then not registering the forces to the foot typically diverges a few CMC time-windows earlier than with registering, but in both cases tracking fails. You can buy a few more time steps before solution diverge irrecoverably by using this transformation "trick". It is not, however, a better answer. If your RRA/CMC tracking is diverging and you are squeezing out a few more time steps, you still have a poor solution and warrants further refinement by increasing tracking weights or having reserve actuators or accepting larger residuals.

Pros and Cons to removing this option:

Some of the reasons to remove it:

  1. confusing for users to comprehend
  2. slows down tools like ID, SO, etc.. because of loading and filtering motion data twice
  3. more often than not produces inconsistent results
  4. is error prone
  5. intended for one obscure use-case
  6. because of recent improvements to ExternalLoads handling (#2332) making the change now eliminates the complexity/additional implementation required to preserve it

and reasons to keep it:

  1. preserving users existing workflows
  2. prevent other code/GUI changes close to release

@opensim-org/dev-team please voice your opinions/views to help us consider the best course of action. Thanks!

aseth1 commented 5 years ago

@aymanhab how difficult is it to remove the "Kinematics for external loads" option and related field from the GUI?

tkuchida commented 5 years ago

Perhaps hide in the GUI but provide access in XML (similar to report_marker_locations in InverseKinematicsTool) for backwards compatibility while promoting best practices for future studies?

nickbianco commented 5 years ago

Honestly, I probably couldn't have explained that option before reading this.

For analyses that already require input kinematics, would a checkbox option like "Re-express external loads into applied body frame" ever make sense? For example, if user wanted to output the re-expressed external loads from inverse dynamics to plot COP in the foot frame.

aseth1 commented 5 years ago

@tkuchida I agree with that more gradual approach and was advocating to do that earlier this year. However, we're now at a point in the development where we're moving the responsibility of loading external loads files to the ExternalLoads (now a Component) for more conistency between API and GUI and that will also allow ExternalLoads to be used via the API and Scirpting, instead of being a utility class for the Tools.

In that respect, removing the option makes that transition almost trivial. If we preserve it, we have to move functionality in the Tools (loading the kinematics, filtering, etc...) into ExternalLoads. Its an investment in time that I frankly do not believe is worth making since there are no actual gains to the user community by preserving it.

chrisdembia commented 5 years ago

I'm copying my feedback from #2332.

My preference is to keep the kinematics option but I am fine with removing it if we are willing to consider adding it back if removing the option substantially prevents users from moving to 4.0.

aseth1 commented 5 years ago

2332 now implements a band-aid fix that maintains the earlier behavior of the Tool reading in the kinematics for external loads which doesn't fix the inconsistency that the GUI still cannot find it without itself doing a change of directory maneuver. That is why the file for the option appears red when you edit the ExternalLoads in the GUI. If you just run the Tool, it just works.

At this stage, we can do as @tkuchida suggests and hide the option in the GUI (and no red field to see) which provides a backdoor via the setup file to have the option. This will at least preserve previous results while discouraging its use going forward when creating new ExternalLoads. The only issue I have with this is that if you start with an existing external loads file that uses this option, then you won't know about it and we are perpetuating its use and silently affecting the user's workflow/results. If we go this route, we should add a message to indicate that option can be executed but the default should run without the option, IMHO.

The message could be something like: "Kinematics for External Loads was detected and can be used to transform external forces into the local reference frame of the applied-to Frame. This is a legacy option that will not be supported in future releases. Proceed without transforming external forces?" [No (legacy only)] [Yes]

The main point is that proceeding without transforming should be the default behavior going forward.

tkuchida commented 5 years ago

The ONLY reason to register forces using input kinematics instead of just applying them in the lab frame is to "mitigate or reduce" drift of the RRA and particular CMC solutions which are tracking those kinematics.

Another potential reason to retain the option is if users have used it for some other reason, or as a shortcut. For example, if external forces were measured on an object that was moving but whose position was not measured (e.g., a rugby tackle bag), could one use this option to always apply the forces to the hand? I vaguely recall something like this in one of the workshops but I can't remember whether this strategy worked.

aymanhab commented 5 years ago

We agreed to stay with the band aid solution leave the option available even if the file may show in red while editing) so that users with old files that specify the option are not stranded. @aseth1 Updated the comment to indicate that this option will be removed.

aseth1 commented 5 years ago

@tkuchida I am not sure I follow the use case. In the case that the force on some pads (say at the shoulder) are measured but the precise location was not measured, you could specify the applied to frame to be any Body/PhysicalFrame including an offset added to the Torso. This would be easier (IMHO) than making up an approximate point in ground and using the option to map the point to the Torso body. Perhaps I am missing something?

tkuchida commented 5 years ago

@aseth1 IIRC Dario was trying to compensate for incomplete measurements. I think he may have had force measurements from a rugby tackle bag, measured at its rigid frame (whose location was tracked) and wanted to apply those forces to the body but through the padding (the deformation of which was not tracked directly). I can't remember the details or whether he ended up using a strategy that exploited the option (IIRC it was at the March workshop, so we could check the slides). My point was only that someone might be using the option in some way we haven't considered—though if they're just shooting themselves in the foot anyway, it would make sense to take the bullets out of the gun.

aymanhab commented 4 years ago

Failures:

aymanhab commented 4 years ago

so_diffs.pdf so_force_diffs.pdf

Static Optimization Activation and force diffs vs standard

aymanhab commented 4 years ago

Will need to account for different column labels as well as the fact that the time range/window is different. @chrisdembia can you comment. Thanks

aymanhab commented 4 years ago

Here's a more meaningful diff between results on branch vs master for CMC test case cmc_master_vs_branch.pdf

jenhicks commented 4 years ago

Thanks @aymanhab . These differences seem more reasonable. Did you ever figure out why the kinematic curves are so different for the standard?

aymanhab commented 4 years ago

Diffs for IAA IAA_master_vs_branch.pdf Diffs SO std vs branch SO_std_vs_branch.pdf

aymanhab commented 4 years ago

@jenhicks I had to manually change the labels since one of them was old format while the other was new format so it's possible I made a mistake in manually editing the labels that caused an offset at some point, would be hard to reconstruct but I can retry if you feel warranted. I guess two separate issues here: 1. is the change from master to branch reasonable, 2. is the test meaningful, if not what to change. Let's confirm 1. then address 2.

aymanhab commented 4 years ago

@aseth1 I have a branch that removes the property kinematics_of_external_loads and the corresponding filtering option, as well as a port of the fix to IAA that you approved earlier which is now necessary. There are three test failures in: testStaticOptimization, testIAA, testCMCWithControlConstraintsGait2354 Attached above are the differences between master and branch for the cases that fail. Can you comment if you believe these are reasonable diffs? Thank you.

aymanhab commented 4 years ago

@aseth1 Hope you'll have a chance to look at the plots above and give us feedback to move ahead with this PR if possible. Thank you.

aymanhab commented 4 years ago

@jenhicks and @carmichaelong This issue contains the before and after files after removal of kinematics for external loads

jenhicks commented 4 years ago

@aymanhab I am OK with updating the standard. Per our discussion, can you let us know the tolerances?

carmichaelong commented 4 years ago

I'm also OK with updating the standard. Comparison look very good for the CMC and SO solutions.

Overall, IAA looks pretty good too, though the total X and Z accelerations do change a little bit (and now indicate that the person isn't quite in steady state). I wonder if the CMC solution that the test is run against used the external loads option?

aymanhab commented 4 years ago

@jenhicks For testRunningModel(CMC) states tolerances are 0.6 except for velocities at 0.2 StaticOptimization activation tol = 0.025, Forces 2.5 IAA tol = 0.15

jenhicks commented 4 years ago

Thanks @aymanhab A tolerance of 0.6 seems pretty big for activations (CMC). Am I understanding correctly?

aymanhab commented 4 years ago

Actually the test that is failing is not testCMCWithControlConstraintsRunningModel which I mentioned above but testCMCWithControlConstraintsGait2354 which has tolerance of .02 and velocity tolerance at .25 https://github.com/opensim-org/opensim-core/blob/8a4171e248890a0fcdbb40ec80814ab90d465245/Applications/CMC/test/testCMCWithControlConstraintsGait2354.cpp#L83 Sorry about the mixup.

jenhicks commented 4 years ago

Thanks ... that seems more reasonable. I'm OK with changing the standard. But why are they so high for the other test?

aymanhab commented 4 years ago

The other test compares kinematics separately but comparison of states have been like this since the test was split off from another test in 2014.

jenhicks commented 4 years ago

OK. Thanks

aseth1 commented 4 years ago

I looked at the latest results and here are some observations and thoughts:

  1. SO: any differences in the application of external forces will have a direct affect on residuals, so the results seem consistent with the differences between ik kinematics (external loads) vs rra kinematics (analysis input). The FX residual has a range of ~20 N and the difference is about 0.4 N (less than 2%) and the muscle activation are within the test tolerance. In this case the tolerance on the residuals could be slightly increased.

  2. CMC: the situation is similar to SO in that there is a inconsistency between desired kinematics for tracking and for external loads. We use the same source (file) but external loads filters the kinematics whereas CMC is not filtering. It isn't surprising that the subtalar and mtp kinematics (speeds) or most affected. Going forward it is probably best to update the standard and keep (or tighten) the tolerances since the previous results were a product of inconsistent settings and probably not a good idea to keep it as a standard.

  3. IAA: there are a couple of aspects that seem improved in the PR results. First, in Sam's 2010 paper there was an inexplicable "gap" of about 5% in the muscle contributions to total acceleration vs GRF immediately after initial contact. For the total Y, in particular, this initial transition looks a lot better. Second, the individual muscle contributions are less "spikey" particularly for the glutes and ankle evertors/invertors, which were inexplicable before. Unfortunately, there is no reduction in FZ contribution to mediolateral acceleration (which is the largest of all residuals). To improve the consistency of the IAA results we would also need to perform RRA and CMC without kinematics for external loads, but I doubt the conclusions would change. It would be fine with me to update the standard. I recommend we attempt to isolate fixes to IAA first. I suggest creating a test case test case in 3.3 where kinematics for external loads is not used and to verify the differences are the same as we are seeing in 4.x. Establishing the "standard" in 3.3 would at least regress 4.x updates to 3.3, which is still highly used, while providing a more accurate test.

jenhicks commented 4 years ago

@aymanhab Can you add a note opensim-core changelog (gui is already updated)?

aseth1 commented 3 years ago

We should also update documentation, which indicates using kinematics for external loads: e.g. https://simtk-confluence.stanford.edu/display/OpenSim/How+to+Use+the+Inverse+Dynamics+Tool

jenhicks commented 3 years ago

Agreed .... we could go ahead and edit the live page now (I think there is a submodule on the page that pulls the same info about external loads into all the relevant tools). The doc will be covering 4.0 and beyond ... so the doc should say "As of OpenSim 4.2 ...."

aymanhab commented 3 years ago

A note has been added to the ExternalLoads page on confluence.