opensim-core icon indicating copy to clipboard operation
opensim-core copied to clipboard

No warning/error if an unexpected XML tag is used

Open carmichaelong opened this issue 3 years ago • 4 comments

From the forum, a user noted that changing the output_precision tag in the XML file does not change the precision of the output file in ID: https://simtk.org/plugins/phpBB/viewtopicPhpbb.php?f=91&t=14074&p=0&start=0&view=&sid=9e745b6d243f57bfa1ba0ac8bdad1106

Checking the code, this looks to likely also be true for other tools as well. Testing with the IK files from gait10dof18musc, I also couldn't get output_precision to change the output file precision.

Some thoughts chatting with @aymanhab:

  • 8 decimal places should be enough for most anyone, unless there's some really small model out there that I can't think of?
  • Could remove the property
  • Or could try to make property work (in case there are use cases out there that need the extra precision, or could reduce file size for some users if they want fewer decimal places)

carmichaelong avatar Oct 28 '21 21:10 carmichaelong

@aymanhab Update on this that could change some decisions. output_precision is a property in AbstractTool, from which AnalyzeTool, CMCTool, ForwardTool, and RRATool derive. Testing with RRATool, I could change the property and see a difference in number of digits in the output.

output_precision is NOT a property in Tool, from which InverseDynamicsTool, IMUInverseKinematicsTool, and InverseKinematicsTool derive, so this matches what was seen by the user that this doesn't work for ID, and also for my testing in IK.

Questions/Comments:

  • Any reason that both AbstractTool and Tool exist and don't derive from one another?
  • We should at least make the behavior the same. At this point, I'm leaning towards making sure that output_precision is included in Tool

carmichaelong avatar Dec 06 '21 21:12 carmichaelong

Thanks for the investigation @carmichaelong. Answers

  1. History: AbstractTool came first and was used as base class for all Tools, then IK, ID were completely rewritten in 2017 based on Tool/DynamicsTool with the intention to replace AbstractTool but that never materialized! The newer Tools use the new Property system while older Tools don't.
  2. Going with the hypothesis that output_precision is unnecessary, I'd avoid reintroducing it anywhere, instead I'd remove it and see if issues arise. I can explain more of the history if needed.

aymanhab avatar Dec 07 '21 18:12 aymanhab

After discussion with @aymanhab, this is a wider problem that there is no feedback for a user when an incorrect XML tag is given for an object. Would be very helpful for users to give them a warning/error when an XML tag is unused (due to error) so that it's easier to debug.

carmichaelong avatar Jan 05 '22 00:01 carmichaelong

:+1: Related: #686

tkuchida avatar Jan 05 '22 00:01 tkuchida