opensim-org / opensim-gui

SimTK OpenSim graphical user interface and distribution.
Apache License 2.0
63 stars 33 forks source link

User interface for "Kinematics for external loads" textbox is misleading #348

Closed tkuchida closed 3 years ago

tkuchida commented 7 years ago

Issue arose in topic 8066 on the Forum. The textbox can be left blank (in fact, the Confluence documentation suggests leaving it blank) but the user interface has not been designed this way. Perhaps the textbox should be disabled by default, enabled by clicking a checkbox, and renamed "Override kinematics for external loads". See detail provided by the user on the Forum, linked above.

jimmyDunne commented 7 years ago

I would like to reiterate Tom's comments. This 'feature' has been very confusing for the community. @aseth1 and I have spoken previously— he suggested getting rid of this all together. I vote 🔥

jenhicks commented 7 years ago

I agree ... let's make this a priority for OpenSim 4.1

aseth1 commented 4 years ago

I wish this had been removed in 4.1! Every student has been confused by it.

aseth1 commented 4 years ago

Also it is not clear that Filter kinematics box is only associated with these kinematics. You do not have to provide Kinematics to click the check box and set a value. The filtering option should also be removed.

aymanhab commented 4 years ago

Absolutely, sorry this fell thru the cracks. Will move to backlog at the next dev meeting. Thank you for the reminder @aseth1 👀

aymanhab commented 4 years ago

Removing from the dialog is straightforward, easily done, how does the API handle old files, or do we leave a back-door vs. remove completely?

jenhicks commented 4 years ago

Will remove completely and give users a warning.

aymanhab commented 4 years ago

Removing the option/properties from API/core results in the following failures, associated with the warning that kineamtics_for_external_loads are unsupported: 75:testInducedAccelerations 77:testStaticOptimization 90:testCMCWithControlConstraintsGait2354

jenhicks commented 4 years ago

@aymanhab Did you resolve this? It sounds like we need to update the set-up files for these tests. Other than the warnings do the tests pass (i.e., produce the reference values)?

aymanhab commented 4 years ago

@jenhicks not yet, the files are actually used (one of them is Sam Hamner's running model). I'm looking into the failures to see what's involved. If these are hard/impossible to reproduce we can go to a plan B where the options are shown but disabled in the GUI and stay as is for API.

jenhicks commented 4 years ago

What do you mean by "the files are actually used"?

aymanhab commented 4 years ago

Example: testStaticOptimization has externalForces file that used to specify kinematics for externalLoads. By removing this reference and treating forces as in ground, the force point of application is different and the results are different more than the test tolerance.

aymanhab commented 4 years ago

Diffs in SO test case testModelWithPassiveForces: Activation differences SOPassiveDiffs I wouldn't consider this functionally different since the residuals (FX) are meant to absorb any small changes in force application. Force differences (example) SOPassiveDiffsForces

jenhicks commented 4 years ago

Thanks for the update @aymanhab . It would be helpful to see all of the results for all of the affected test cases. @nickbianco or @chrisdembia do you have any scripts that would help with generating these kind of summary plots for us to look at?

aymanhab commented 4 years ago

testInducedAcceleration fails due to a bug where it looks for "ground" in the BodySet and fails because ground is not a Body in 4.0+

aymanhab commented 4 years ago

After a fix, results are generally similar but test fails, e.g. IAADiffsCOM

jenhicks commented 4 years ago

I looked into the IAA set-up. In Sam's published simulations shared on simtk, he does not specify a separate kinematics file for the external loads when running IAA. So I'm not sure why it is specified here. https://simtk.org/projects/nmbl_running

aymanhab commented 4 years ago

@jenhicks I had the impression that our testInducedAcceleration was based on Sam's analysis/work because it uses a model called subject02_running_RRA_cycle02_sim02_07_v232 but I can't confirm if/where this is used in Sam's published data. I don't actually know for sure if any of his files would run as is out of the box in version 4.0+. Sorry about the confusion.

jenhicks commented 4 years ago

@aymanhab I was looking into the published results, because I was wondering if there was a scientific reason for including the kinematics for external loads. It seems since Sam didn't include them, that there is not. The files in the test are named a little differently, so I'm not sure of their exact provenance.

chrisdembia commented 4 years ago

The test failing due to "ground" makes more sense than the test failing due to different results, because IAA shoud not alter kinematics (or, perhaps I do not know enough about IAA; perhaps adding the foot-ground constraints results in different kinematics).

After a fix, results are generally similar but test fails, e.g.

What does this graph compare? The new results should be nearly identical to the results on master, because IAA does not allow kinematics to vary.

jenhicks commented 4 years ago

@chrisdembia The kinematics are not different when posing the model for to run the IAA. But how the ground reaction forces are applied is different. @aymanhab Are you plotting the sum of accelerations induced by all actuators (per unit force)? Or what exactly?

aymanhab commented 4 years ago

@chrisdembia as @jenhicks explained it's the ExternalLoad application that is different.

@jenhicks the plot is for the first quantity that the test compares against standard and fails (center of mass induced acceleration total_X, _Y,_Z). I assume if the totals are different then the decomposition due to different muscles will be different as well. There are more comparisons downstream in the test but I wanted to communicate the general flavor of the changes.

chrisdembia commented 4 years ago

@jenhicks https://github.com/opensim-org/opensim-moco/blob/master/Moco/plot_trajectory.py This script can plot any Storage file. Must have the OpenSim Python package available.

jenhicks commented 4 years ago

@chrisdembia @carmichaelong and I discussed this today. We are leaning towards changing the reference. But first, we should plot all the results for reference vs. running with current master vs. running with the proposed PR.

aymanhab commented 4 years ago

Thanks @jenhicks will do and follow up.

aseth1 commented 4 years ago

testInducedAcceleration fails due to a bug where it looks for "ground" in the BodySet and fails because ground is not a Body in 4.0+

A fixed was proposed in #2691 that seems acceptable.

jenhicks commented 4 years ago

https://github.com/opensim-org/opensim-core/issues/2334#issuecomment-643417537