Iterator Matlab Class for easier access #1851

jimmyDunne · 2017-08-01T22:01:11Z

Fixes issue

Deals with #1465 and effects #1436

Brief summary of changes

A Matlab class for doing basic operations with OpenSim Lists. This is a utility for working with lists on Matlab, allowing similar operation as the Sets.

Testing I've completed

Basic operation (does it return the correct things). I would just like to show implementation before building a proper test.

Looking for feedback on...

I have spent a few hours mocking this up and seeing if it is at all feasible. I would like feedback on (i) using Matlab classes instead of a function (ii) the interface, how does the implementation look (seen in testOsimList), (iii) any spec requests (additions or deletions).

I can also make a DevProp if requested?

This change is

tkuchida · 2017-08-01T22:29:57Z

An interface like this could be really helpful for MATLAB users who are unfamiliar with C++ iterators. I'm wondering whether get() would be slow if, e.g., used with a large model or within an optimizer. For example, if scale factors were being set on every Body in the Model, then get() would be performing n(n+1)/2 string comparisons and calls to eval() (which is notoriously slow) on each iteration. One strategy to avoid this would be to cache a map between the string and a reference (UUIDs would be useful here 😉)—though, to avoid using out-of-date information, the struct would then need to detect changes to the Model and regenerate the map.

chrisdembia

Something like this would be extremely helpful to users. It's great you included a test. We should make use of this in our example files so people know how to use it.

I think we could improve the clarity of the interface a little more. I think there are two different ways in which users want to access subcomponents: looping through components of a specific type, and getting a specific single component by name.

Right now, these two concepts seem to be intertwined unnecessarily. If you want to get a single component by name, you don't need the ComponentLists. Just use your eval()-getConcreteClassName() trick (however, this will not always work; some classes have weird names I think).

This class does not really seem to support iterating through components in a simpler fashion (e.g., by index).

My proposal is:

Change get() to take an index rather than a name, thereby allowing one to use this class to iterate through components.
Perhaps add a getByName() but this use should be discouraged.
Add a separate function to Utilities named osimGetComponent() or osimGetComp() etc. that fetches a single component and downcasts it for you. The signature would be osimGetComponent(model, path).

We should discuss in a dev meeting.

chrisdembia · 2017-08-02T23:40:07Z

Bindings/Java/Matlab/Utilities/osimList.m

+            elseif nargin == 1
+                error('constructor takes two inputs, not one')
+            elseif nargin > 2
+                error(['2 inputs required, num2str(nargin) ' given])


The num2str(nargin) is inside the string, and therefore will not be evaluated. I think you intended to place it outside the string.

Bindings/Java/Matlab/Utilities/osimList.m

chrisdembia · 2017-08-02T23:42:23Z

Bindings/Java/Matlab/Utilities/osimList.m

+            while ~li.equals(list.end())
+                if strcmp(char(li.getName()),name)
+                    pathname = li.getAbsolutePathName();
+                    comp = model.getComponent(pathname);


You already have the component in the list; no need to fetch it out of the model.

Oh now I think I remember...it doesn't quite work. Well it should be an easy fix to the swig interface files to make that work. I think the issue is described here ("MATLAB methods can't start with underscore muscle = muscleIter.deref();").

Looking at the iterator methods, it is not clear then how to return a reference to a particular object.

Why can't you return li since it is the Component you are querying?

@aseth1 the type of li is something like ComponentListIterator<Component>. It is not the actual component and we should hide it from the user.

We could add a fix to the SWIG interface as a performance enhancement sometime in the future.

aymanhab · 2017-08-03T18:08:11Z

My opinion is that it's a good start but really this should be done on the opensim-core side or at worst in SWIG bindings side. Forcing Matlab users to use lists and iterating is cumbersome, error-prone and counter-intuitive as far as I can tell. We should create methods to build Sets (or any other preferred container) of proper type and return them. Most scripters are using pre-made models so cached, properly-typed containers will make such an easy task easy while the iterators allow more involved model building/modification type of tasks possible.

tkuchida · 2017-08-03T19:40:20Z

We should create methods to build Sets (or any other preferred container) of proper type and return them.

Potentially relevant discussion in Forum topic 8086. There, the suggestion was

"As of 4.0 Beta API, we're discouraging the use of BodySet, JointSet etc. as they may not provide a complete list of Bodies, Joints, ... since the model structure is potentially hierarchical, instead you'd use model.getComponent("PathName") and then safeDowncast to proper type (if you need to)."

I'm unclear about how we're expecting users to interact with the new API. These expectations may not align with what's most natural.

💡 Also note that Model::getNumBodies() returns the size of the BodySet, so if we're discouraging use of the BodySet because it might contain incomplete information, Model::getNumBodies() shouldn't be using it.

aymanhab · 2017-08-03T19:50:38Z

Good point @tkuchida, my answer on the forum though was in the context of the current (what's available today) rather than what's the ideal 4.0 final API. The forum post goes to highlight the issues with the interface as it stands. Thank you.

tkuchida · 2017-08-03T20:12:48Z

my answer on the forum though was in the context of the current (what's available today) rather than what's the ideal 4.0 final API. The forum post goes to highlight the issues with the interface as it stands. Thank you.

Ok, thanks for clarifying. I actually encountered this issue in my own code yesterday and I was confused because I thought use of BodySet, JointSet, etc. were now discouraged so I was surprised to see them used internally by getNumBodies(), etc. Might be good to update those methods.

…tors

jimmyDunne · 2018-10-15T18:07:28Z

@aseth1, if you could review and let me know. I would like to write some documentation for using lists— perhaps we could chat (Zoom)?.

…tors

aymanhab · 2018-10-16T18:27:34Z

Per discussion in stand-up the problem is caused by the ' deref()' method not available in Matlab. Will look into the swig bindings and create an alias that's allowable (since other code already uses deref we don't want to just rename the method).

…available in matlab

aymanhab · 2018-10-16T21:31:08Z

@jimmyDunne deref() method added, and should show up for all iterators, please let me know if that works for you.

…org/opensim-core into matlabScripts_iterators

jimmyDunne · 2018-10-17T16:51:37Z

Thanks, @aymanhab! That made the Matlab implementation doable.

This seems to work well now.

jimmyDunne · 2018-10-17T17:06:34Z

@aseth1, should be good for a re-review 👍

aseth1

LGTM. I made some comments that might make it more helpful.

aseth1 · 2018-10-17T18:24:38Z

Bindings/Java/Matlab/Utilities/osimList.m

@@ -69,25 +89,36 @@
            end
            outputnames = outputnames';
        end
-        function reference = get(obj,name)
+        function reference = getByName(obj,name)


Do Matlab style comments after the method appear if you type help classname.methodname in Matlab? If so, it might be nice to describe the method and input arguments.

aseth1 · 2018-10-17T18:25:25Z

Bindings/Java/Matlab/Utilities/osimList.m

-                    comp = model.getComponent(pathname);
-                    class = comp.getConcreteClassName();
-                    eval(['reference = ' char(class) '.safeDownCast(comp);'])
+                    reference = li.deref;


aseth1 · 2018-10-17T19:25:24Z

Bindings/Java/Matlab/Utilities/osimList.m

+                error(['Component name not found: ' name])
+            end
+        end
+        function reference = getByIndex(obj,index)


This seems fine for getting a single object by index, but I'm not sure we would want to then use this method to iterate through the list by index. You could imagine a user doing this:

for i = 1: osimList.getSize() comp = osimList.getByIndex(i); comp.setSomePropertyValue(val); end

which essentially iterates through the same ComponentList numerous times. Instead if we could access the internal list directly we could do something like:

list = osimList.getList() li = list.begin() while ~li.equals(list.end()) li.next(); li.setSomePropertyValue(val); end

which would be more efficient.

FWIW, I would expect most MATLABers to use a for loop by default. To avoid inefficiency, might it be possible to keep track of the last item that was retrieved and start searching from there (essentially doing li.next() under the covers until list.end(), then starting from the beginning)? Alternatively, perhaps just documentation (with the above warning and code snippets) would be helpful under the getByIndex() method.

Thanks @tkuchida, @aseth1, and @chrisdembia for the awesome comments! This design feedback is great— I have been spending some time thinking about the philosophy of the list design and how that conflicts with the typical Matlab user for-loop strategy. The original reason I wanted to make this was to make it easier for me to get a single component from a list without having to write a while loop everytime. Perhaps it would be better to simplify this class to a set of static methods that are for single use only;

% Get the number of components in the model nComps = osimList.getNumberOfComponents(model, 'Body') % Get all the body names (as strings) bNames = osimList.getNames(model, 'Body') % I only want to work on the Pelvis, so I get it directly. pelvis = osimList.getComponent(model, 'Body', names{i} )

This wouldn't stop anyone from using it in a for-loop, but we could write in documentation, and in the help section of the m-file, when to use the above method and when to use the list directly (when you want to deal with most components in the list).

I am happy to jump on a call to discuss since I feel this may need discussion.

aseth1 · 2018-10-17T19:25:31Z

Bindings/Java/Matlab/Utilities/osimList.m

+
+classdef osimList < matlab.mixin.SetGet
+% osimList(classname)
+%   Use OpenSim lists to iterate through model components


Perhaps a clearer description would be:

OsimList provides convenient access to a Model's components listed according to type by name or index. To make edits to all items in a list, consider iterating through a ComponentList directly.

chrisdembia

I made a few comments. This will help users. I wouldn't worry about optimizing the interface/implementation of this class, because as we have discussed, a long term solution will be in C++.

chrisdembia · 2018-10-17T17:55:53Z

Bindings/Java/Matlab/Utilities/osimList.m

+%   OpenSim Utility Class
+%   Inputs:
+%   model               Reference to OpenSim Model (Model())
+%   classname           OpenSim component class name ('Body', 'Muscle' 


Missing a parenthesis?

chrisdembia · 2018-10-17T17:56:32Z

Bindings/Java/Matlab/Utilities/osimList.m

+            elseif nargin == 1
+                error('constructor takes two inputs, not one')
+            elseif nargin > 2
+                error(['2 inputs required, ' num2str(nargin) 'given'])


Suggested change

error(['2 inputs required, ' num2str(nargin) 'given'])

error(['2 inputs required, ' num2str(nargin) ' given'])

chrisdembia · 2018-10-17T17:57:38Z

Bindings/Java/Matlab/Utilities/osimList.m

+                eval(['list = model.get' classname 'List();']);
+                disp('List creation Successful');
+            catch 
+                error(['OpenSim classname, ' classname ', does not exist']);


It's not necessarily that the classname does not exist, it's that there is no Model::getXList().

chrisdembia · 2018-10-17T17:58:11Z

Bindings/Java/Matlab/Utilities/osimList.m

+                eval(['list = model.get' classname 'List();']);
+                disp('List creation Successful');
+            catch 
+                error(['OpenSim classname, ' classname ', does not exist']);


Suggested change

error(['OpenSim classname, ' classname ', does not exist']);

error(['Class ' classname ' not supported by osimList']);

chrisdembia · 2018-10-17T17:59:37Z

Bindings/Java/Matlab/Utilities/osimList.m

+            while ~li.equals(list.end())
+                if strcmp(char(li.getName()),name)
+                    reference = li.deref;
+                    break


Suggested change

break

return

chrisdembia · 2018-10-17T17:59:56Z

Bindings/Java/Matlab/Utilities/osimList.m

+            end 
+            if ~exist('reference','var')
+                error(['Component name not found: ' name])
+            end


Could remove the if here and just create the error (if you return in the loop).

…tors

jimmyDunne · 2018-10-19T17:21:05Z

@chrisdembia and @aseth1, once we agreed on generating a Matlab cell array of references, it was better to turn this into a simple function. Now the user inputs a model and a classname and the function returns a Matlab cell array of references. The user can then make their own decisions on how to deal with the cell array.

I added a test (which is more of an example for now) and ran it locally (it passed)

aseth1 · 2018-10-19T17:56:53Z

Now the user inputs a model and a classname and the function returns a Matlab cell array of references. The user can then make their own decisions on how to deal with the cell array.

👍

chrisdembia

Looks great :) I made 2 minor comments. Thanks for dealing with the back and forth.

Bindings/Java/Matlab/Utilities/osimList2MatlabCell.m

chrisdembia · 2018-10-19T18:07:47Z

Bindings/Java/Matlab/tests/testOsimList2MatlabCell.m

+import org.opensim.modeling.*
+
+%% Instantiate a model from file
+model = Model('../../../../OpenSim/Tests/shared/arm26.osim');


Because arm26.osim is copied into the test directory, you can simply use 'arm26.osim' here. We don't want to read files from the repository in the test case.

chrisdembia · 2018-10-19T18:08:31Z

Bindings/Java/Matlab/tests/testOsimList2MatlabCell.m

+import org.opensim.modeling.*
+
+%% Instantiate a model from file
+model = Model('../../../../OpenSim/Tests/shared/arm26.osim');


Suggested change

model = Model('../../../../OpenSim/Tests/shared/arm26.osim');

model = Model('arm26.osim');

jimmyDunne added 2 commits August 1, 2017 14:34

Matlab class for osim iterators

adaa9d2

comments

8c84ac8

jimmyDunne changed the title ~~{WIP} Iterator Matlab Class for easier access~~ [WIP] Iterator Matlab Class for easier access Aug 1, 2017

chrisdembia reviewed Aug 2, 2017

View reviewed changes

jimmyDunne added 2 commits October 10, 2018 13:11

Merge remote-tracking branch 'origin/master' into matlabScripts_itera…

c69a1f5

…tors

Merge remote-tracking branch 'origin/master' into matlabScripts_itera…

ae3d00e

…tors

jimmyDunne self-assigned this Oct 10, 2018

scrum2a added this to the 20181001 milestone Oct 10, 2018

chrisdembia modified the milestones: 20181001, 20181015 Oct 12, 2018

Merge remote-tracking branch 'origin/master' into matlabScripts_itera…

5811e41

…tors

Add method to get deref rather than operator since the latter is not …

a8d5663

…available in matlab

jimmyDunne added 2 commits October 17, 2018 09:45

Merge branch 'matlabScripts_iterators' of https://github.com/opensim-…

1f1f70d

…org/opensim-core into matlabScripts_iterators

updated matlab class with dref method

ade574b

update test

ff44f1b

aseth1 reviewed Oct 17, 2018

View reviewed changes

chrisdembia reviewed Oct 17, 2018

View reviewed changes

jimmyDunne added 3 commits October 18, 2018 13:35

Methods are now static

4fdc1a9

fixes

ea632ba

Merge remote-tracking branch 'origin/master' into matlabScripts_itera…

8e5f5f0

…tors

jimmyDunne added 2 commits October 19, 2018 13:03

Changed to cell array function

1531b91

added test

91d95f1

chrisdembia requested changes Oct 19, 2018

View reviewed changes

chrisdembia reviewed Oct 19, 2018

View reviewed changes

updated from feedback

a43f11e

jimmyDunne merged commit 083fcc4 into master Oct 19, 2018

jimmyDunne deleted the matlabScripts_iterators branch October 19, 2018 20:01

jimmyDunne changed the title ~~[WIP] Iterator Matlab Class for easier access~~ Iterator Matlab Class for easier access Oct 19, 2018

	error(['2 inputs required, ' num2str(nargin) 'given'])
	error(['2 inputs required, ' num2str(nargin) ' given'])

	error(['OpenSim classname, ' classname ', does not exist']);
	error(['Class ' classname ' not supported by osimList']);

	model = Model('../../../../OpenSim/Tests/shared/arm26.osim');
	model = Model('arm26.osim');

Iterator Matlab Class for easier access #1851

Iterator Matlab Class for easier access #1851

Conversation

jimmyDunne commented Aug 1, 2017 • edited by sherm1 Loading

Fixes issue

Brief summary of changes

Testing I've completed

Looking for feedback on...

tkuchida commented Aug 1, 2017

chrisdembia 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

aymanhab commented Aug 3, 2017

tkuchida commented Aug 3, 2017

aymanhab commented Aug 3, 2017

tkuchida commented Aug 3, 2017

jimmyDunne commented Oct 15, 2018

aymanhab commented Oct 16, 2018 • edited Loading

aymanhab commented Oct 16, 2018

jimmyDunne commented Oct 17, 2018 • edited Loading

jimmyDunne commented Oct 17, 2018

aseth1 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

jimmyDunne Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia 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

jimmyDunne commented Oct 19, 2018

aseth1 commented Oct 19, 2018

chrisdembia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyDunne commented Aug 1, 2017 •

edited by sherm1

Loading

aymanhab commented Oct 16, 2018 •

edited

Loading

jimmyDunne commented Oct 17, 2018 •

edited

Loading

jimmyDunne Oct 18, 2018 •

edited

Loading