search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
722 stars 38 forks source link

[REVIEW]: MocoExtendProblem: Interface Between OpenSim and MATLAB for Rapidly Developing Direct Collocation Goals in Moco #7110

Open editorialbot opened 3 months ago

editorialbot commented 3 months ago

Submitting author: !--author-handle-->@Aravind-Sundararajan<!--end-author-handle-- (Aravind Sundararajan) Repository: https://github.com/Aravind-Sundararajan/MocoExtendProblem Branch with paper.md (empty if default branch): Version: v1.0.0 Editor: !--editor-->@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @0todd0000, @aasadi1, @dgupta7 Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/ab4ece70adece3811308955d52be6a2f"><img src="https://joss.theoj.org/papers/ab4ece70adece3811308955d52be6a2f/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/ab4ece70adece3811308955d52be6a2f/status.svg)](https://joss.theoj.org/papers/ab4ece70adece3811308955d52be6a2f)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@0todd0000 & @aasadi1, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @Kevin-Mattheus-Moerman know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @0todd0000

📝 Checklist for @dgupta7

📝 Checklist for @aasadi1

editorialbot commented 3 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 3 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1137/16M1062569 is OK
- 10.1371/journal.pcbi.1008493 is OK
- 10.1002/cnm.3777 is OK
- 10.1098/rsif.2019.0402 is OK
- 10.1371/journal.pcbi.1010466 is OK
- 10.1371/journal.pcbi.1006223 is OK
- 10.1186/s12984-023-01279-5 is OK
- 10.1007/s10107-004-0559-y is OK

MISSING DOIs

- No DOI given, and none found for title: Optimal control gait simulations of older adults p...
- No DOI given, and none found for title: Optimal Control Simulations of 3-D Walking in Huma...
- No DOI given, and none found for title: mexplus

INVALID DOIs

- None
editorialbot commented 3 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.69 s (1404.6 files/s, 318777.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
XML                             86              0           1585         111339
HTML                           422           2958           2105          80451
JavaScript                     320            186            265           6773
C/C++ Header                    50            471           1193           3063
CSS                              4            419            107           2159
MATLAB                          29            456            868           1840
C++                             32            358            392           1292
Markdown                         4            115              0            285
SVG                             16              0              2            263
TeX                              1             17              0            124
CMake                            1             11              4             97
YAML                             1              0              5             30
-------------------------------------------------------------------------------
SUM:                           966           4991           6526         207716
-------------------------------------------------------------------------------

Commit count by author:

   144  Aravind-Sundararajan
    93  AspectWaltz
    38  Varun Joshi
    11  Aravind Sundararajan
editorialbot commented 3 months ago

Paper file info:

📄 Wordcount for paper.md is 1414

✅ The paper includes a Statement of need section

editorialbot commented 3 months ago

License info:

🟡 License found: GNU General Public License v3.0 (Check here for OSI approval)

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

0todd0000 commented 3 months ago

Review checklist for @0todd0000

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Kevin-Mattheus-Moerman commented 3 months ago

@Aravind-Sundararajan, @0todd0000, @aasadi1, this is where the review takes place. Let me know if you have any questions.

Kevin-Mattheus-Moerman commented 2 months ago

@editorialbot add @dgupta7 as reviewer

editorialbot commented 2 months ago

@dgupta7 added to the reviewers list!

Kevin-Mattheus-Moerman commented 2 months ago

@0todd0000, @aasadi1, @dgupta7 this is where the review takes place. Thanks for your help. To get started you can post the following in a comment here @editorialbot generate my checklist. This will generate your checklist to guide you through the review. I stay on top of things I suggest you monitor your GitHub notifications regularly, and the check in there. Thanks!

dgupta7 commented 2 months ago

Review checklist for @dgupta7

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

0todd0000 commented 2 months ago

@Kevin-Mattheus-Moerman Should I link to all issues in this thread or is it OK to just create issues and not reference them in this thread? I have just submitted a setup issue and it seems redundant to also mention this issue here. Please clarify, thanks!

Kevin-Mattheus-Moerman commented 2 months ago

@0todd0000 if it is not too much trouble then please do link to them here. If there are too many then just link to the main ones. This is just so we have a bit of a track record of the review steps. Thanks for your help!

aasadi1 commented 2 months ago

Review checklist for @aasadi1

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Kevin-Mattheus-Moerman commented 2 months ago

@0todd0000, @aasadi1, @dgupta7 I hope you are getting on well with this review. If you are able to provide a bit of an update that would be great. If there are minor issues you want the author to address you can mention them here, if there are major things you can link to issues raised on the project repository. Thanks again for your help!

dgupta7 commented 2 months ago

Review of the written paper. Review_DG.pdf

dgupta7 commented 2 months ago

Review of README.md Note: I understand that some of the details I request here would be in the paper, but for a software, people will likely first read the ReadMe, so I assume that adding more details in ReadMe is important. Additionally, this comment is based only on reading the ReadMe file. I haven't started the installation process yet.

Summary:

Setup & Requirements:

Getting Started: Creating MEX interface:

Testing:

License: I am wondering GPL license is being used and not MIT? MIT gives more freedom to the users.

dgupta7 commented 2 months ago

When I started installing the software, I typed mex -setup C++ in MATLAB command prompt and it gave me the error "Error using mex Supported compiler not found. You can install the freely available MinGW-w64 C/C++ compiler; see Install MinGW-w64 Compiler. For more options, visit https://www.mathworks.com/support/compilers."

I am wondering what is the reason. Maybe because while installing visual studio I only installed the "Desktop development with C++" workload. Do we need more than that? VS

Aravind-Sundararajan commented 2 months ago

When I started installing the software, I typed mex -setup C++ in MATLAB command prompt and it gave me the error "Error using mex Supported compiler not found. You can install the freely available MinGW-w64 C/C++ compiler; see Install MinGW-w64 Compiler. For more options, visit https://www.mathworks.com/support/compilers."

I am wondering what is the reason. Maybe because while installing visual studio I only installed the "Desktop development with C++" workload. Do we need more than that? VS

that should be enough. Ensure that cl.exe can be run from cmd or powershell, or that your vs version is added to the system PATH environment variable

dgupta7 commented 2 months ago

How do I ensure that cl.exe can be run from cmd? or how do I ensure that vs version is added to the system PATH environment variable? My VS 2022 community does not have a clear bin folder to add to the system paths. Another thing I tried was to start matlab from x64 Native Command Prompt, and then run the build.m in MEP, but it still gave me the error that it cannot find the compiler.

It is possible that I am doing something stupid but I think that installation problems will be common among users and more clear documentation here is needed to help users with exactly what to do step-by-step to get MEP setup correctly.

Aravind-Sundararajan commented 2 months ago

How do I ensure that cl.exe can be run from cmd? or how do I ensure that vs version is added to the system PATH environment variable? My VS 2022 community does not have a clear bin folder to add to the system paths. Another thing I tried was to start matlab from x64 Native Command Prompt, and then run the build.m in MEP, but it still gave me the error that it cannot find the compiler.

It is possible that I am doing something stupid but I think that installation problems will be common among users and more clear documentation here is needed to help users with exactly what to do step-by-step to get MEP setup correctly.

you can check in verbose mode where matlab is looking for vs2019 or 2022, mex -setup CPP -v. I don't remember if you need to install a package for mex support when installing matlab so it's possible something is missing that I don't know.

I have this in my path environment variable C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin

aasadi1 commented 1 month ago

I have the same problem. VS is in my path environment: C:\Program Files\Microsoft Visual Studio\2022\Community\Msbuild\Current\Bin

but yet when I run cl.exe in the terminal I get this error:

cl.exe : The term 'cl.exe' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again. At line:1 char:1

Aravind-Sundararajan commented 1 month ago

I have the same problem. VS is in my path environment: C:\Program Files\Microsoft Visual Studio\2022\Community\Msbuild\Current\Bin

but yet when I run cl.exe in the terminal I get this error:

cl.exe : The term 'cl.exe' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again. At line:1 char:1

* cl.exe

* ```
    + CategoryInfo          : ObjectNotFound: (cl.exe:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException
  ```

Do you mean the build.m script or just visual studio's cl.exe? The matlab script should work if you can call msbuild and cmake from terminal.

aasadi1 commented 1 month ago

I downloaded CMake and installed it an rant build.m and now I get this error:

CMake Deprecation Warning at CMakeLists.txt:1 (cmake_minimum_required): Compatibility with CMake < 3.5 will be removed from a future version of CMake.

Update the VERSION argument value or use a ... suffix to tell CMake that the project does not need compatibility with older versions.

CMake Error at CMakeLists.txt:2 (project): Running

'nmake' '-?'

failed with:

no such file or directory

CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage -- Configuring incomplete, errors occurred! MSBuild version 17.11.9+a69bbaaf5 for .NET Framework MSBUILD : error MSB1009: Project file does not exist. Switch: C:\Users\azara\Downloads\MocoExtendProblem-main\MocoExtendProblem-main\build\customGoals.sln detected using opensim 4.4 or lower. Error using fprintf Invalid file identifier. Use fopen to generate a valid file identifier.

Error in build_extend_class (line 29) fprintf(fid,'%s\n',s);

Error in build (line 44) build_extend_class(fullfile(bindir,config,cppName),fullfile(bindir,config,wrapName), opensim_install);

Also I changed OpenSim 4.5 to 4.4 in the code.

Aravind-Sundararajan commented 1 month ago

gi

I downloaded CMake and installed it an rant build.m and now I get this error:

CMake Deprecation Warning at CMakeLists.txt:1 (cmake_minimum_required): Compatibility with CMake < 3.5 will be removed from a future version of CMake.

Update the VERSION argument value or use a ... suffix to tell CMake that the project does not need compatibility with older versions.

CMake Error at CMakeLists.txt:2 (project): Running

'nmake' '-?'

failed with:

no such file or directory

CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage -- Configuring incomplete, errors occurred! MSBuild version 17.11.9+a69bbaaf5 for .NET Framework MSBUILD : error MSB1009: Project file does not exist. Switch: C:\Users\azara\Downloads\MocoExtendProblem-main\MocoExtendProblem-main\build\customGoals.sln detected using opensim 4.4 or lower. Error using fprintf Invalid file identifier. Use fopen to generate a valid file identifier.

Error in build_extend_class (line 29) fprintf(fid,'%s\n',s);

Error in build (line 44) build_extend_class(fullfile(bindir,config,cppName),fullfile(bindir,config,wrapName), opensim_install);

Also I changed OpenSim 4.5 to 4.4 in the code. image

looks like it wants to use nmake makefiles instead of Visual Studio 2022 or 2019, if you call cmake -G you'll get a list of generators available.

you can try changing the call on line 29 to 

system("cmake CmakeLists.txt -S . -B """+builddir+""" -DOSim_Version=4 -G ""Visual Studio 17 2022""");

thereby specifying the generator. There are also ways to change the default generator too but you'll have to look that up as I'm not too familiar.

aasadi1 commented 1 month ago

just did:

Unrecognized function or variable 'builddir'.

Error in build_extend_class (line 31) system("cmake CmakeLists.txt -S . -B """+builddir+""" -DOSim_Version=4 -G 'Visual Studio 17 2022'");

Error in build (line 44) build_extend_class(fullfile(bindir,config,cppName),fullfile(bindir,config,wrapName), opensim_install);

Aravind-Sundararajan commented 1 month ago

just did:

Unrecognized function or variable 'builddir'.

Error in build_extend_class (line 31) system("cmake CmakeLists.txt -S . -B """+builddir+""" -DOSim_Version=4 -G 'Visual Studio 17 2022'");

Error in build (line 44) build_extend_class(fullfile(bindir,config,cppName),fullfile(bindir,config,wrapName), opensim_install);

I think you've changed a line in the wrong file, you should stash changes and pull again as I have updated the repo with the desired change

aasadi1 commented 1 month ago

I finally got it installed. Maybe you wanna add this part to the readme file for people like me who are not familiar with software development.

Open Visual Studio Go to Tools -> Get Tools and Features In the "Workloads" tab enable "Desktop development with C++" Click Modify at the bottom right

Also I think you should ask the user to change the VS and OpenSim version in the build.m file like we did.

dgupta7 commented 1 month ago

I was able to run build.m on a different computer. I actually did not need VS bin folder in system PATH and cl.exe still does not run from command prompt. Are those actually required for build.m, or maybe needed later? One notable difference was that on this other computer I used MATLAB 2022b, I was using 2021a earlier. MEX -setup C++ itself was not working on the computer with 2021a. Does MATLAB version matter for MEP? if yes, please add that to the documentation.

dgupta7 commented 1 month ago

I did the following to further the review process.

  1. I followed the instructions to create a new goal. I copied the MocoActivationGoal. I wanted to add the functionality of exponent of 3 and 4. I called it MocoCustomGoal. The directions are not very clear. It's not just the file names that have to be changed, but the names of classes and other variables might also need to be changed, depending on application. It also seems to me that the user needs to know how to use OpenSim API to actually make new custom goals. It seems that the user might need to have some knowledge of C++ as well. It would be nice to also add what qualifications or softwares the user needs to know to use MEP. Additionally, if the user already understands the OpenSim API, it should be added briefly how they benefit from MEP, that is, API does not provide this option.

  2. In the test_ExtendProblem_simple.m, the opensimroot is hard coded. It needs to be user specified. The file also gives an error since it needs to be run from the top directory and not from inside the folder 'test'. I think for OpenSim verion lower than 4.5, the _compat file is to be used. This is not mentioned anywhere. I was able to finally run the example and generate the figure that looks similar to the one in the readme section. Why is my figure not the same as the readme section (I have attached my figure here)? What do I need to do to replicate the result in ReadMe? My biggest problem with this example is that I have no idea what I ran. There is no file header for the user to understand what the code does. what does this example do? what are the task constraints? what is the goal here? what does the max coordinate goal do? image

  3. I ran the driver.m but again the filepaths are incorrect. The files need to be run form the top directory. If they need to be run from the top directory, why are they placed in the test directory? Which custom goals of MEP do the Walksim_Tracking.m and WalkSim_predictive.m use? What is the advantage, that is, what makes these simulations unique such that they cannot be used with regular Moco? I think that these information will help highlight the power of MEP. I was looking for the MocoActivationGoal to be used in the walking simulations but I only saw the MocoControlGoal, which is already a part of Moco, I want to use the new goal I created (in point 1) where I would penalize the activation^3 and activation^4. I thought that I would replace MocoActivationGoal with my custom goal to test the MEP. Could you add that possibility? I would also like more explanation of the tracking and predictive simulations of walking. Is it 2D or 3D? The header of the WalkSim_predictive_compat.m says that it is a tracking problem. What is the cost function here?

  4. When I ran driver.m, the tracking part failed. I got an error saying "Error using WalkSim_Tracking tracking failed to match reference output for goal". Why would that happen? An example code should not fail. What do I do to fix this? Also please provide reference results, that is, what should the results of running driver.m look like (for both tracking and predictive simulation) so that the user can be satisfied that they have installed everything correctly.

  5. I think that if more documentation can be added to each of the new goals that are created in MEP, that will already be a great addition as people can just use those goals in their Moco. For example, I was excited to see the new Goals that are there in the custom_goals_compat folder, but I have no idea what each of them do and how to use them.

I think that overall the MEP is very useful and can be highly impactful but it needs far better documentation for people to be able to use it.

Aravind-Sundararajan commented 1 month ago

I did the following to further the review process.

1. I followed the instructions to create a new goal. I copied the MocoActivationGoal. I wanted to add the functionality of exponent of 3 and 4. I called it MocoCustomGoal. The directions are not very clear. It's not just the file names that have to be changed, but the names of classes and other variables might also need to be changed, depending on application. It also seems to me that the user needs to know how to use OpenSim API to actually make new custom goals. It seems that the user might need to have some knowledge of C++ as well. It would be nice to also add what qualifications or softwares the user needs to know to use MEP. Additionally, if the user already understands the OpenSim API, it should be added briefly how they benefit from MEP, that is, API does not provide this option.

2. In the test_ExtendProblem_simple.m, the opensimroot is hard coded. It needs to be user specified. The file also gives an error since it needs to be run from the top directory and not from inside the folder 'test'.  I think for OpenSim verion lower than 4.5, the _compat file is to be used. This is not mentioned anywhere. I was able to finally run the example and generate the figure that looks similar to the one in the readme section. Why is my figure not the same as the readme section (I have attached my figure here)? What do I need to do to replicate the result in ReadMe? My biggest problem with this example is that I have no idea what I ran. There is no file header for the user to understand what the code does. what does this example do? what are the task constraints? what is the goal here? what does the max coordinate goal do?
   ![image](https://private-user-images.githubusercontent.com/85564166/369887539-1e0d969b-bf1b-4d04-80ff-cb7c1f287018.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjcxMDAxNzAsIm5iZiI6MTcyNzA5OTg3MCwicGF0aCI6Ii84NTU2NDE2Ni8zNjk4ODc1MzktMWUwZDk2OWItYmYxYi00ZDA0LTgwZmYtY2I3YzFmMjg3MDE4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTIzVDEzNTc1MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJhZmUwZDdkN2RhZTViODc3YjM0YWVkNzkyYWQ4MDMxM2NhYmJhMWNmYzVjODk4ODIxZjYxYzE0MmUwOWE4NWEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.IjePRXkby0F3x_Mig1Ly-mXmnLyLkeK0hKcZBhtWVbE)

3. I ran the driver.m but again the filepaths are incorrect. The files need to be run form the top directory. If they need to be run from the top directory, why are they placed in the test directory? Which custom goals of MEP do the Walksim_Tracking.m and WalkSim_predictive.m use? What is the advantage, that is, what makes these simulations unique such that they cannot be used with regular Moco? I think that these information will help highlight the power of MEP. I was looking for the MocoActivationGoal to be used in the walking simulations but I only saw the MocoControlGoal, which is already a part of Moco, I want to use the new goal I created (in point 1) where I would penalize the activation^3 and activation^4. I thought that I would replace MocoActivationGoal with my custom goal to test the MEP. Could you add that possibility? I would also like more explanation of the tracking and predictive simulations of walking. Is it 2D or 3D? The header of the WalkSim_predictive_compat.m says that it is a tracking problem. What is the cost function here?

4. When I ran driver.m, the tracking part failed. I got an error saying "Error using WalkSim_Tracking
   tracking failed to match reference output for goal". Why would that happen? An example code should not fail. What do I do to fix this? Also please provide reference results, that is, what should the results of running driver.m look like (for both tracking and predictive simulation) so that the user can be satisfied that they have installed everything correctly.

5. I think that if more documentation can be added to each of the new goals that are created in MEP, that will already be a great addition as people can just use those goals in their Moco. For example, I was excited to see the new Goals that are there in the custom_goals_compat folder, but I have no idea what each of them do and how to use them.

I think that overall the MEP is very useful and can be highly impactful but it needs far better documentation for people to be able to use it.

Let me first address some of the issues that can be solved by updating the readme/ manuscript and then discuss modifications to the scripts themselves

Aravind-Sundararajan commented 1 month ago

I did the following to further the review process.

1. I followed the instructions to create a new goal. I copied the MocoActivationGoal. I wanted to add the functionality of exponent of 3 and 4. I called it MocoCustomGoal. The directions are not very clear. It's not just the file names that have to be changed, but the names of classes and other variables might also need to be changed, depending on application. It also seems to me that the user needs to know how to use OpenSim API to actually make new custom goals. It seems that the user might need to have some knowledge of C++ as well. It would be nice to also add what qualifications or softwares the user needs to know to use MEP. Additionally, if the user already understands the OpenSim API, it should be added briefly how they benefit from MEP, that is, API does not provide this option.

2. In the test_ExtendProblem_simple.m, the opensimroot is hard coded. It needs to be user specified. The file also gives an error since it needs to be run from the top directory and not from inside the folder 'test'.  I think for OpenSim verion lower than 4.5, the _compat file is to be used. This is not mentioned anywhere. I was able to finally run the example and generate the figure that looks similar to the one in the readme section. Why is my figure not the same as the readme section (I have attached my figure here)? What do I need to do to replicate the result in ReadMe? My biggest problem with this example is that I have no idea what I ran. There is no file header for the user to understand what the code does. what does this example do? what are the task constraints? what is the goal here? what does the max coordinate goal do?
   ![image](https://private-user-images.githubusercontent.com/85564166/369887539-1e0d969b-bf1b-4d04-80ff-cb7c1f287018.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjc3MzE1NTUsIm5iZiI6MTcyNzczMTI1NSwicGF0aCI6Ii84NTU2NDE2Ni8zNjk4ODc1MzktMWUwZDk2OWItYmYxYi00ZDA0LTgwZmYtY2I3YzFmMjg3MDE4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTMwVDIxMjA1NVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTg1YmY5MWIxYjhjYmRmYzA2MjhmZjIwMjBkNDRhNTIxN2MwZjY2ZDdhMDM5MmNjYjU2NDQ3MDgxMzZjOTg1NDYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.nb-Pe-N6AOu0H0iJR3BGtaDe5h536LRuawtckko-Qfo)

3. I ran the driver.m but again the filepaths are incorrect. The files need to be run form the top directory. If they need to be run from the top directory, why are they placed in the test directory? Which custom goals of MEP do the Walksim_Tracking.m and WalkSim_predictive.m use? What is the advantage, that is, what makes these simulations unique such that they cannot be used with regular Moco? I think that these information will help highlight the power of MEP. I was looking for the MocoActivationGoal to be used in the walking simulations but I only saw the MocoControlGoal, which is already a part of Moco, I want to use the new goal I created (in point 1) where I would penalize the activation^3 and activation^4. I thought that I would replace MocoActivationGoal with my custom goal to test the MEP. Could you add that possibility? I would also like more explanation of the tracking and predictive simulations of walking. Is it 2D or 3D? The header of the WalkSim_predictive_compat.m says that it is a tracking problem. What is the cost function here?

4. When I ran driver.m, the tracking part failed. I got an error saying "Error using WalkSim_Tracking
   tracking failed to match reference output for goal". Why would that happen? An example code should not fail. What do I do to fix this? Also please provide reference results, that is, what should the results of running driver.m look like (for both tracking and predictive simulation) so that the user can be satisfied that they have installed everything correctly.

5. I think that if more documentation can be added to each of the new goals that are created in MEP, that will already be a great addition as people can just use those goals in their Moco. For example, I was excited to see the new Goals that are there in the custom_goals_compat folder, but I have no idea what each of them do and how to use them.

I think that overall the MEP is very useful and can be highly impactful but it needs far better documentation for people to be able to use it.

I've hopefully addressed most of the issues in the document and added a tutorial section on goal creation. As for automating new goal creation, IMO this is beyond the scope of the project and copy-paste and a regex replace or Preserve Case replace in vscode is pretty simple enough.

As for tests sequestered in the test directory, I opt to add tests to the matlab path and this is a convention I use that is not abnormal; However, MEP can be used from anywhere if you add MocoExtendProblem/bin/RelWithDebInfo to the matlab path. I instead opt to addpath(genpath(fullfile(pwd,'bin','RelWithDebInfo'))) while staying on the top-level directory. Just a manner of taste.

And now let me address some of the issues with the examples/test files

aasadi1 commented 3 weeks ago

I finished my review. I was able to run the test without any errors. I did not edit any custom goals as I believe they need C++ knowledge. Last part says that:

"Now feel free to modify MocoCustomGoal.cpp and MocoCustomGoal.h for new goals! For help reference the OpenSim API reference documentation."

So we need toedit the .cpp or .h files to adjust the goal right? I was under the impression that this project is basically for those that only know MATLAB. I'm not sure someone like me with only MATLAB knowledge can adjust the .cpp and .h files to create custom goals. If it's possible, it needs more documentation on how to do that.

The paper looks really good and informative. Great job on it.

Let me know if I need to do anything else.

Aravind-Sundararajan commented 3 weeks ago

I finished my review. I was able to run the test without any errors. I did not edit any custom goals as I believe they need C++ knowledge. Last part says that:

"Now feel free to modify MocoCustomGoal.cpp and MocoCustomGoal.h for new goals! For help reference the OpenSim API reference documentation."

So we need toedit the .cpp or .h files to adjust the goal right? I was under the impression that this project is basically for those that only know MATLAB. I'm not sure someone like me with only MATLAB knowledge can adjust the .cpp and .h files to create custom goals. If it's possible, it needs more documentation on how to do that.

The paper looks really good and informative. Great job on it.

Let me know if I need to do anything else.

Hi thanks. Some understanding of the OpenSim API and C++ is necessary to develop new custom goals but MEP serves as a build tool and solution besides rebuilding OpenSim from scratch and the associated bindings or making a custom plugin. Some additional tools could be developed like something that automatically makes a template custom goal based on an input name, but I opted not to do so as it seemed out of the scope of the current tool.

Most of the issues people have seem to be with naming conventions though so if it seems necessary, I could spend some time making such a tool.

Kevin-Mattheus-Moerman commented 3 weeks ago

@0todd0000, @dgupta7 please can you resume your review as well?

@aasadi1 can you respond to @Aravind-Sundararajan 's comment?

Kevin-Mattheus-Moerman commented 1 day ago

@0todd0000, @dgupta7 @aasadi1 :wave: