Upgrade InverseKinematicsTool to new Property system #2685

aymanhab · 2020-03-05T19:22:01Z

Fixes issue #<issue_number>

Brief summary of changes

Convert inversekinematicstool to use new property system, other cleanup. Kept old API around

Testing I've completed

Ran tests

Looking for feedback on...

CHANGELOG.md (choose one)

no need to update because...
updated...

The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.

This change is

…gress

…to all other object registrations

…racking orientations

…f only tracking orientations" This reverts commit ca8ccdc.

aymanhab · 2020-03-16T20:04:55Z

Ready for review @carmichaelong when you have a chance

carmichaelong

Thanks, @aymanhab! I've left a bunch of minor comments (some just clarifications for my understanding).

carmichaelong · 2020-03-19T21:20:49Z

OpenSim/Tools/InverseDynamicsTool.cpp

@@ -274,7 +274,6 @@ bool InverseDynamicsTool::run()
                    << _lowpassCutoffFrequency << "..." << endl << endl;
                _coordinateValues->pad(_coordinateValues->getSize()/2);
                _coordinateValues->lowpassIIR(_lowpassCutoffFrequency);
-                if (getVerboseLevel()==Debug) _coordinateValues->print("coordinateDataFiltered.sto");


Is this a loss of functionality for someone trying to debug?

True but rarely used AFAIK, we're moving in the direction of having a logger that does logging across all objects/tools so a private verbosity level flag is very ad-hoc and can be removed.

Sounds good. Is there an issue open about what the plans are for the logger? Would be good to make sure these are in there if so.

PR #2551 is the new logger PR

carmichaelong · 2020-03-19T21:44:47Z

OpenSim/Tools/InverseKinematicsTool.cpp

@@ -318,10 +163,10 @@ bool InverseKinematicsTool::run()
        // Adjust the time range for the tool if the provided range exceeds
        // that of the marker data.
        SimTK::Vec2 markersValidTimRange = markersReference.getValidTimeRange();


I know that it's not part of the PR, but could be good to make the change of markersValidTimRange ->markersValidTimeRange

carmichaelong · 2020-03-19T22:01:54Z

OpenSim/Tools/InverseKinematicsTool.h

-    void setStartTime(double d) { _timeRange[0] = d; };
-    double getStartTime() const {return  _timeRange[0]; };
+    void setStartTime(double d) { upd_time_range(0) = d; };
+    double getStartTime() const {return  get_time_range(0); };


I didn't find anything about conventions for one-liners in headers files in our contributing file, but it would be good to at least be consistent with spacing through this section.

Does the format file that @chrisdembia included take care of this? If not I will update since I agree with your suggestion

Unsure, do you know where that file is and how it works? If so, I could try to look into it, but in the mean time might just be good to clean up this section?

carmichaelong · 2020-03-19T22:02:27Z

OpenSim/Tools/Tool.h

-    Tool() : _inputsDir(_inputsDirProp.getValueStr()),
-        _resultsDir(_resultsDirProp.getValueStr())
-        { setNull(); };
+    Tool() { constructProperties();


Put constructProperties() on own line?

carmichaelong · 2020-03-19T22:04:48Z

OpenSim/Tools/InverseKinematicsTool.cpp

@@ -511,9 +359,7 @@ void InverseKinematicsTool::updateFromXMLNode(SimTK::Xml::Element& aNode, int ve
                for (; p!= trialIter->node_end(); ++p) {
                    iter->insertNodeAfter( iter->node_end(), p->clone());
                }
-                iter->insertNodeAfter( iter->node_end(), Xml::Comment(_constraintWeightProp.getComment()));


Just to double check, the comments will still be in the files due to the way the new property system works, correct?

Yes, the goal is to construct an object with the correct property values, comments on the other hand are never read so setting them here is unnecessary

carmichaelong · 2020-03-19T22:05:29Z

OpenSim/Tools/InverseKinematicsTool.h

@@ -22,10 +22,10 @@
 * See the License for the specific language governing permissions and        *
 * limitations under the License.                                             *
 * -------------------------------------------------------------------------- */
-
+#include "Simbody.h"


I think this is only needed for SimTK::ReferencePtr? Can the include be of smaller scope?

That is true, I followed suit with similar usage because headers that defines templates are tricky to surgically include but I can look into it further if you think it's worth it.

Will #include <SimTKcommon/internal/ReferencePtr.h> work?

carmichaelong · 2020-03-19T22:09:22Z

OpenSim/Tools/InverseKinematicsTool.h

-    double &_accuracy;
+    OpenSim_DECLARE_PROPERTY(accuracy, double,
+            "The accuracy of the solution in absolute terms, i.e. the number of "
+            "significant digits to which the solution can be trusted. Default 1e-6.");


I think the default is 1e-5?

carmichaelong · 2020-03-19T22:33:20Z

OpenSim/Tools/InverseKinematicsTool.h

-    PropertyDblArray _timeRangeProp;
-    Array<double> &_timeRange;
+    OpenSim_DECLARE_PROPERTY(coordinate_file, std::string,
+            "The name of the storage (.sto or .mot) file containing the time "


Can this be a path too?

We don't have a path property, feel free to propose one 😉 All file names in xml are string properties AFAIK

carmichaelong · 2020-03-19T22:34:19Z

OpenSim/Tools/InverseKinematicsTool.h

-    PropertyStr _markerFileNameProp;
-    std::string &_markerFileName;
+    OpenSim_DECLARE_PROPERTY(marker_file, std::string,
+            "Name/path to a .trc or .sto file of type Vec3 of marker data.");


Many users using the GUI or files won't know what "type Vec3" will mean. How can we convey the necessary info to that type of user?

May also be useful to keep some of the original comment, but reword, so that users have context for this. For instance: Markers in this " "file that have a corresponding task and model marker are included." This comment isn't perfect, but it's getting at something useful about having a corresponding IK task a model marker aligned with each of the experimental markers.

Good point regarding comments. I basically had to consolidate comments from old InverseKinematicsTool and IMUInverseKinematicsTool which have same properties with different comments. In this case I used the one from the more recent IMUInverseKinematicsTool but I agree the old InverseKinematicsTool comments were better,were the remaining properties clear or should I revert all comments?

I think the others were alright, this one definitely stuck out the most. Probably shouldn't revert all comments.

carmichaelong · 2020-03-19T22:56:43Z

OpenSim/Tools/InverseKinematicsTool.cpp

-
-    return(*this);
+    constructProperty_model_file("");
+    constructProperty_constraint_weight(Infinity);


Is Infinity defined from Simbody? What's the rationale on switching this from std::numeric_limits<SimTK::Real>::infinity()?

Infinity is SimTK::Infinity and as such is much cleaner than the more complicated syntax that returns same value (unless you're aware of other potential side-effects) it was used in other code already.

aymanhab · 2020-03-20T20:15:40Z

@carmichaelong Thanks for the thorough review, I believe I addressed your feedback, let me know what you think when you have a chance. Thank you

aymanhab · 2020-03-24T18:41:32Z

Ready for another pass @carmichaelong Thank you

carmichaelong · 2020-03-24T22:55:14Z

I left a few comments on the open threads. Overall it's just a few very minor things:

Make sure it's documented that the future logger should replace the capability removed here.
Check if the more specific #include works
Clean up white space on some of the one liners.

Merging should be easy after these are addressed since they're pretty minor.

aymanhab · 2020-03-25T16:46:30Z

ok, changes/updates made per your request @carmichaelong Thank you

carmichaelong · 2020-03-25T16:47:57Z

Thanks @aymanhab, I'll keep an eye out for the tests completing

aymanhab · 2020-03-25T21:39:08Z

Testing finished 😃

carmichaelong · 2020-03-25T21:51:00Z

Thanks, and sorry, wanted to make sure to at least put a note about the logging somewhere (I put it in the other PR for now).

Separately, I know you've already continued work from this branch, so I didn't want to delete the unify_iktools branch from opensim-core yet. Feel free to delete the branch if you're all good to go.

aymanhab · 2020-03-25T22:04:29Z

Thanks @carmichaelong I'll rebase my branch on master and then delete this branch. Thank you

aymanhab added 8 commits February 26, 2020 16:20

Remove _verbose level as it doesn't belong to Tool

30c5d90

Remove unused _inputDir property

15681e2

Upgrade Tool to use new property system

23f84b8

Use ReferencePtr instead of naked model pointer

c9d70c0

Upgrade InverseKinematicsTool to use new property system, work in pro…

c9d3989

…gress

Reorder, initialize all Properties

eb87734

Remove registerTypes method and explicit call, should happen similar …

b898d8c

…to all other object registrations

Add orientation_file property and lock translational coords if only t…

ca8ccdc

…racking orientations

aymanhab changed the title ~~[WIP] Unify iktools~~ Upgrade InverseKinematicsTool to new Property system Mar 16, 2020

Revert "Add orientation_file property and lock translational coords i…

c333c2a

…f only tracking orientations" This reverts commit ca8ccdc.

carmichaelong requested changes Mar 19, 2020

View reviewed changes

Address feedback on PR, in particular property comments, variable names

af28d78

Fix quotes

8ab69b2

aymanhab added 2 commits March 25, 2020 01:35

Address feedback on PR, formatting and headers

aef12b3

Move include line later to fix compilation errors

c722623

carmichaelong mentioned this pull request Mar 25, 2020

Integrate spdlog #2551

Open

carmichaelong merged commit 60dd70d into master Mar 25, 2020

aymanhab deleted the unify_iktools branch April 16, 2020 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade InverseKinematicsTool to new Property system #2685

Upgrade InverseKinematicsTool to new Property system #2685

aymanhab commented Mar 5, 2020 •

edited by aseth1

Loading

aymanhab commented Mar 16, 2020

carmichaelong left a comment

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 24, 2020

aymanhab Mar 25, 2020

carmichaelong Mar 19, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 24, 2020

carmichaelong Mar 19, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 24, 2020

carmichaelong Mar 19, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 19, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

carmichaelong Mar 24, 2020

carmichaelong Mar 19, 2020

aymanhab Mar 20, 2020

aymanhab commented Mar 20, 2020

aymanhab commented Mar 24, 2020

carmichaelong commented Mar 24, 2020

aymanhab commented Mar 25, 2020

carmichaelong commented Mar 25, 2020

aymanhab commented Mar 25, 2020

carmichaelong commented Mar 25, 2020

aymanhab commented Mar 25, 2020

Upgrade InverseKinematicsTool to new Property system #2685

Upgrade InverseKinematicsTool to new Property system #2685

Conversation

aymanhab commented Mar 5, 2020 • edited by aseth1 Loading

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

aymanhab commented Mar 16, 2020

carmichaelong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aymanhab commented Mar 20, 2020

aymanhab commented Mar 24, 2020

carmichaelong commented Mar 24, 2020

aymanhab commented Mar 25, 2020

carmichaelong commented Mar 25, 2020

aymanhab commented Mar 25, 2020

carmichaelong commented Mar 25, 2020

aymanhab commented Mar 25, 2020

aymanhab commented Mar 5, 2020 •

edited by aseth1

Loading