search

perfanalytics / pose2sim

Markerless kinematics with any cameras — From 2D Pose estimation to 3D OpenSim motion
https://perfanalytics.github.io/pose2sim/
BSD 3-Clause "New" or "Revised" License
242 stars 46 forks source link

JOSS Review Checklist - Comments and requests #9

Closed jonmatthis closed 2 years ago

jonmatthis commented 2 years ago

I'm just going to leave a list of comments here rather than making a new issue for each thing, which seems like it blow up the number of issues and be hard to track and manage.

I'll use this issue as a rolling place to add my comments as I go. I'll make it clear when I'm "Done" with my first pass through (it might be a few days from this initial post)

Link to my review checklist comment in the official review thread - https://github.com/openjournals/joss-reviews/issues/4362#issuecomment-1115354966

jonmatthis commented 2 years ago

Repository: Is the source code for this software available at the https://gitlab.inria.fr/perfanalytics/pose2sim?

Obviously the repo was moved to this Github location :P

jonmatthis commented 2 years ago

Installation

Do they only need to use the "Windows Portable Demo?"

If so, you should specify that specifically. The link you sent will prompt many newbies to try to go through the full installation process for OpenPose (speaking from experience when I first discovered OpenPose in 2017 😅 )

If you DO need a full installation of OpenPose to run the software, you should specify that users CANNOT just use the Windows Portable Demo

jonmatthis commented 2 years ago

Installation cont

people will miss that step, install things to to their (base) install and then have a devil of a time figuring out why.

I mean, they'll do that regardless, but at least this way it'll be their fault :)

jonmatthis commented 2 years ago

Installation (cont)

Your instructions currently make it look like you should install Pose2Sim from pip and THEN build from source. You should specify that it is one or the other

jonmatthis commented 2 years ago

Minor -

When possible, Python based projects should follow the PEP8 style guide - https://peps.python.org/pep-0008/

In particular, your pip install references Pose2Sim which is not using the recommended capitalization scheme. Technically you should name your pypi project pose2sim

(But feel free not to bother, just pointing out that you're not PEP8 compliant)

jonmatthis commented 2 years ago

Installation

the command pip show Pose2Sim | grep Location does not work on my computer because it doesn't recognize the grep command.

It seems there should be a clearer way to ask uses to cd in to the relevant location

jonmatthis commented 2 years ago

Open a python session in your Pose2Sim\Demo folder, and test the following code:

Statements like this are inscrutable to new-comers. Give them specific instructions on how to cd into that folder and start a python session by entering the command python

jonmatthis commented 2 years ago

Installation Note -

I got the demonstration working, but following the Use on your own data is outside of the scope of this review, so my reviews will assume the basic functionality of the code base

jonmatthis commented 2 years ago

Installation

Ok, I've finished the Installation

It's mostly good, but for those relatively minor comments above.

You could help new users a lot by sign-posting that the "Use your own data" instructions are ALSO part of the "demonstation"

As listed, a person will start from the top, work through the demo, and then think that the rest of the ReadMe is for people who have recorded their own data.

You could provide the users with expected output from the "demonstration" commands and then explain that deeper explanation and instructions for loading the Sample data into OpenSim are presented below

also -If your demonstration produces labeled images, it would be best to make those visible to the user (or at least link to where they are located

jonmatthis commented 2 years ago

Ok I'm done with my review!

jonmatthis commented 2 years ago

final comments back on main review thread -

https://github.com/openjournals/joss-reviews/issues/4362#issuecomment-1161853931

davidpagnon commented 2 years ago

Dear Jonathan, Thank you for your review!

Installation

  • Installing OpenPose is notoriously challenging, especially for new users. Do they only need to use the "Windows Portable Demo?" If so, you should specify that specifically. The link you sent will prompt many newbies to try to go through the full installation process for OpenPose (speaking from experience when I first discovered OpenPose in 2017 😅 ) If you DO need a full installation of OpenPose to run the software, you should specify that users CANNOT just use the Windows Portable Demo
  • You should specify which versions of Open Sim have been tested (i.e. `2. Install OpenSim 4.x (tested with v4.3, download-link)

I now specified these:

  1. Install OpenPose (instructions there). Portable demo is enough.
  2. Install OpenSim 4.x (there). Tested up to v4.4-beta.
  • You are missing the step where you tell people how to activate the conda environment with conda activate Pose2Sim people will miss that step, install things to to their (base) install and then have a devil of a time figuring out why.

I don't think I forgot this step, unless I am missing something?

conda create -n Pose2Sim python=3.8.8 
conda activate Pose2Sim

Your instructions currently make it look like you should install Pose2Sim from pip and THEN build from source. You should specify that it is one or the other

I see, I made it clear now that it's two different options.

your pip install references Pose2Sim which is not using the recommended capitalization scheme. Technically you should name your pypi project pose2sim

You are right, and it is case insensitive anyway. Next PyPI release will be named pose2sim (see setup.cfg).

the command pip show Pose2Sim | grep Location does not work on my computer because it doesn't recognize the grep command.

What kind of terminal are you using? I'm a little suprised, as it works with me on linux shell, powershell, anaconda prompt. pip show pose2sim would work, however you would have to find the "Location" by yourself.

I'll post this for now, before I make a mistake and lose all of my answers :sweat_smile:

davidpagnon commented 2 years ago

Open a python session in your Pose2Sim\Demo folder, and test the following code: Statements like this are inscrutable to new-comers. Give them specific instructions on how to cd into that folder and start a python session by entering the command python

This has been made clearer for beginners: \ Open a terminal and check package location with pip show pose2sim | grep Location. \ Copy this path and go to the Demo folder with cd <path>\pose2sim\Demo. \ Open a python session with python command, and test the following code

You could help new users a lot by sign-posting that the "Use your own data" instructions are ALSO part of the "demonstation" As listed, a person will start from the top, work through the demo, and then think that the rest of the ReadMe is for people who have recorded their own data. You could provide the users with expected output from the "demonstration" commands and then explain that deeper explanation and instructions for loading the Sample data into OpenSim are presented below

I made it clearer in the caption of the Demonstration section: \ This demonstration provides an example experiment of a person balancing on a beam, filmed with 4 calibrated cameras processed with OpenPose. Deeper explanations and instructions are given on the section below.

also -If your demonstration produces labeled images, it would be best to make those visible to the user (or at least link to where they are located

My demonstration doesn't provide labeled images, unfortunately my lab's policy doesn't want anyone to be recognizable on a public repository (I actually had the hardest time even being allowed to provide any json files at all.)

I'm going to work on a Maya and/or Blender add-on to get a visual feedback at all stages, but this is out of the scope of this current review.

Thank you for your comments and suggestions!