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]: Scyland3D: Processing 3D landmarks #1262

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: @ybayle (Yann Bayle) Repository: https://github.com/ybayle/Scyland3D Version: v1.1.0 Editor: @Kevin-Mattheus-Moerman Reviewers: @r-barnes, @patrikhuber Archive: 10.5281/zenodo.3637546

Status

status

Status badge code:

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

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) 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

@r-barnes, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @Kevin-Mattheus-Moerman know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @r-barnes

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @patrikhuber

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @r-barnes it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

Kevin-Mattheus-Moerman commented 5 years ago

@r-barnes thanks. This is where the review takes place and you can see the tickboxes at the top here. Note there are also instructions to avoid the spam you mentioned. Let me know if you have questions.

Kevin-Mattheus-Moerman commented 5 years ago

@ybayle this is where the review takes place. Please also note the comments by @patrikhuber over at https://github.com/openjournals/joss-reviews/issues/1153

Kevin-Mattheus-Moerman commented 5 years ago

@r-barnes are you able to start the review process? Thanks

labarba commented 5 years ago

I notice that the .py file mentions MIT License, while the LICENSE file is GNU. https://github.com/ybayle/Scyland3D/blob/1d219d40a8327515d936b94a2d1d4ce7060c0f16/Scyland3D.py#L5

Kevin-Mattheus-Moerman commented 5 years ago

@r-barnes can you please confirm you are able to continue with the review process? Thanks

ybayle commented 5 years ago

I notice that the .py file mentions MIT License, while the LICENSE file is GNU. https://github.com/ybayle/Scyland3D/blob/1d219d40a8327515d936b94a2d1d4ce7060c0f16/Scyland3D.py#L5

Thanks for noticing! We have updated our license consistently accordingly.

r-barnes commented 5 years ago
patrikhuber commented 5 years ago

I am moving my comments from the other thread to here, I think they would be more appropriate here since the other thread is closed.

Overview

It seems to me that in general, this software is for a very, very limited use case (which of course isn't necessarily bad!). Basically, the main thing it does: 1) Reads .pts landmarks from a software called "Landmark software (v3.6)" and converts them to a .csv format 2) It can mirror and re-order those landmarks while converting them. The mirroring potentially seems like it can be a bit tricky (see more below).

Comments/questions:

Scyland3D is a Python tool for converting 3D raw landmark and semilandmark coordinates to a usable format for geometric morphometric analyses

but if you're not from exactly that field, you're going to have a hard time discerning if the tool is useful or not, because "3D raw landmarks" is such a large field. I am not from the author's field directly but "geometric morphometric analysis" looks like it is in fact what I know by the term shape analysis, which is a really broad field, with thousands of different landmark formats. And Sciland3D seems only for one specific task in one very particular field (or perhaps even one particular lab or commercial software), without much broader applicability, I think. At least for me as a computer vision researcher, having worked with statistical shape models a lot, I had a hard time figuring out what this exactly is and does.

To sum that part up, I think there's way more context needed, and the name "Scyland3D: Processing 3D landmarks" may also be too general.

Specific comments regarding the paper (PDF):

Specifically with regards to the code:

Some of those comments might be a bit picky but in particular because I don't think this contribution is particularly big, it is even more important that the small thing that is contributed is as useful as possible and can be used by people being assured it doesn't have any bugs, or if they do need to check something, the code should be documented clearly enough to make it easy.

Good luck and all the best! :-)

r-barnes commented 5 years ago

As far as I can tell there are no tests. Is this a problem?

r-barnes commented 5 years ago

I've completed the JOSS rubric and have raised concerns linked above.

r-barnes commented 5 years ago

(I want to apologize for the delay in getting to this review, I've been experiencing some issues with RSI which is made it difficult to keep up with computer-based tasks. I'm now using a speech recognition program to do the review, so I apologize for any grammatical errors or odd choices of words.)

Many of the issues that I've raised are on the target repository.

I share @patrikhuber's concerns that this piece of software is super specific. When I saw the name of the software and description of the softer, I expected to find a large amount of code relating to the manipulation of 3D points, instead the softer it appears to consist predominantly of what's essentially a utility function for a much larger pipeline. Given the limited scope of the software, I would have difficulty recommending it for publication.

Similar to the other reviewer, I don't see any kind of epsilon cutoff to deal with floating-point issues. I also worry about the utility of the mirroring, it seems as though the user has to specify the plane of the mirror. it seems to me that this is a problem that is better suited to a technique such as iterative closest point or optimal transport.

The code is at times verbose:

coord = []
    for item in data:
        tmp = []
        for xyz in item.split(","):
            tmp.append(float(xyz))
coord.append(tmp)

could be rendered as

coord = [float(c) for item in data for c in item.split(",")]

Granted, such techniques should not be employed where they impede the readability of code, but I think there are several instances in which the existing code is more complex than is necessary. export2csv() might be the best example of this, since Python includes an entire core module for solving exactly this problem.

This contribution is small enough that it should be as perfect and rock-solid as possible. For that, I'm looking for cleaner, better-commented code, test cases, and an exceptionally well-explained use case. Even so, I think this function is just too limited in scope to make a good contribution.

r-barnes commented 5 years ago

@Kevin-Mattheus-Moerman

Kevin-Mattheus-Moerman commented 5 years ago

Thanks @r-barnes for your review work and detailed comments!

Kevin-Mattheus-Moerman commented 5 years ago

Thanks @patrikhuber also for your comments here!

Kevin-Mattheus-Moerman commented 5 years ago

@ybayle can you formulate a detailed reply to the above comments?

ybayle commented 5 years ago

Yes, I have started to fix some issues reported by the reviewers on the main repo! I'll keep you updated.

r-barnes commented 5 years ago

Thanks all, do ping me when I'm next needed.

r-barnes commented 5 years ago

I notice that I've been assigned. Should I review again?

Kevin-Mattheus-Moerman commented 5 years ago

@r-barnes I think we are still waiting for @ybayle to implement changes and work on the issues reported. @ybayle can you provide an update as to where we stand? Thanks!

ybayle commented 5 years ago

Sorry for the delay, I am fixing the issues reported and will get back to you ASAP. I just have to finish something else before. I'll ping when everything will be ready!

labarba commented 5 years ago

👋 @ybayle — Can you give us an update? If you're not close to done, please let me know of a time period to set a reminder for you.

ybayle commented 5 years ago

@labarba: Sorry for the delay, I have taken into account most comments from @r-barnes in the last commits and started processing comments from @patrikhuber locally that will be pushed soon.

ybayle commented 5 years ago

@patrikhuber Thank you for your valuable comments to improve our submission, we detail below our changes.

The Readme needs a better introduction in my opinion. The PDF and repo don't explain or introduce what "semilandmarks" are.

The ReadMe and the paper have been updated accordingly.

The Readme mentions .pts files but not what they are or where they're coming from. We have .pts files too in the facial landmarks community and they are something completely different (e.g. see https://github.com/patrikhuber/eos/blob/master/examples/data/image_0010.pts).

As I can see, we provide as examples nearly the same .pts files except, we have an additional column for values for the z-axis. We added a link in the paper and in the ReadMe to the definition of the .pts file format we use.

"Few free software solutions allow digitizing 3D landmarks and semilandmarks [...]": What do you mean with that? Store them to the disk? I think again what you probably mean is that few ready-made scripts exist that convert between all the many formats?

You raise an interesting question there about the variety of vocabulary used here. We saw in numerous places (e.g. https://www.rdocumentation.org/packages/geomorph/versions/3.1.2/topics/digitize2d or http://people.tamu.edu/~alawing/materials/ESSM689/Quick_Guide_to_Geomorph_v2.0.pdf) the word "digitizing" used for "data acquisition". In order to avoid confusion with other readers we used both words in the paper.

"mirrored objects have reverted bending" What do you mean with bending here? How useful is the mirroring? For it to be useful, you usually have to mirror the original data too (in your case e.g. the teeth point-cloud), in the same way. Furthermore it seems to me that e.g. in the example given in Figure 1, this only works if the landmarks are placed in a symmetric way in the first place (i.e. if 1 is the "mirror-point" of 7, etc.).

We are talking here about the main curvature a surface can display in terms of how much they display a concave or convex shape on a given axis. In our example with shark's teeth, they are longer (x) than wide (y) and very thin (z). Teeth are concave on the z-axis and their tip are not centred relatively to their base. So, when you mirror them to match left teeth to right teeth, you have their base and tip that are overlapping but you one is concave while the other is convex as can be seen on their z-axis. That's why "mirrored objects have reverted bending".

The code could use more documentation in general. A tiny bit longer function documentation, but also in-code comments. Code should be either self-explanatory and need no comments (best case) or have appropriate comments, where needed.

We had the same comment from the second reviewer and updated our code accordingly.

Here it says something about a comparison "up to epsilon": https://github.com/ybayle/Scyland3D/blob/1d219d40a8327515d936b94a2d1d4ce7060c0f16/Scyland3D.py#L80 But in the following code, I can't find any comparison up to an epsilon, only equality checks. It may very well be that I don't read the code correctly and I haven't run it either but it kind of reinforces my point from above about documentation.

Indeed, the first version of the script was using an epsilon as a workaround to report duplicated landmarks in the exported .pts files. A better solution has been found since. The comment is now updated.

ybayle commented 5 years ago

@r-barnes

I also worry about the utility of the mirroring, it seems as though the user has to specify the plane of the mirror. it seems to me that this is a problem that is better suited to a technique such as iterative closest point or optimal transport.

We detailed further our code to explain why the mirroring is needed. We also added details about the fact that our code finds a plane and that no user specified plane has to be supplied.

ybayle commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

patrikhuber commented 5 years ago

Hi @ybayle,

Thanks for addressing the comments! I think it looks much better!

A few things that I think still warrant clarification, or that I've noticed while reviewing the changes:

It still looks to me like your script is very specific, more concretely, it seems specific to the "Landmark 3.6", "Landmark Editor" software from IDAV, UC Davis (http://www.idav.ucdavis.edu/research/EvoMorph). Sure, other programs may export the exact same .pts files (do you know of any such?), but I think it would clarify the context of your script if this was prominently mentioned in the Readme introduction, and it would help people using the "Landmark 3.6" software to find your script and know that it might be useful to them.

I'm not sure if the wording of the second sentence could be a bit confusing. It may be interpreted as a restriction on the MIT License? IANAL but perhaps rewording might make sense.

"might be mixed up" is not very clear/scientific. Is this a particular problem/bug in the "Landmark 3.6" software? It seems to me that if it mixes up landmarks and semilandmarks, that just removing duplicates would not really be a solution?

ybayle commented 5 years ago

Hi @patrikhuber,

Thanks for your comments.

It still looks to me like your script is very specific, more concretely, it seems specific to the "Landmark 3.6", "Landmark Editor" software from IDAV, UC Davis (http://www.idav.ucdavis.edu/research/EvoMorph).

I have updated the ReadMe to prominently mention this in the introduction.

In this image, don't you want to mirror the landmark numbers as well? Otherwise you change the semantic meaning of each landmark? In my field, when mirroring, we want to preserve the semantic meaning of a landmark when mirroring and not reorder them but it might be different in your use field.

Indeed, our script handle the inversion or not of the numbering of the landmarks. Our use-case is closely related to this kind of processing, thus justifying our current example image.

It may be interpreted as a restriction on the MIT License?

I have reworded the license.

"reverts": Not sure I understand. Do you perhaps mean "reverses"?

Thanks you for pointing out this typo.

Do you check here if the x, y and z coordinates are the same, or only one of them?

This sentence has been reworded.

"might be mixed up" is not very clear/scientific.

This has been reworded. Indeed, the landmark acquisition software avoid setting landmarks at the same position but landmarks can be set up at the same position of a semilandmark, which is not wanted.

Also there were a couple of instances of dodgy grammar/wording. I think you might be mixing BE/AE? E.g. "characterizing" vs "favoured".

The paper and ReadMe have been checked for misspelling and now use BE. Please confirm no mistakes or typos are remaining.

ybayle commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

patrikhuber commented 5 years ago

Hi @ybayle,

Looks good!

Indeed, our script handle the inversion or not of the numbering of the landmarks.

I see - but I'm not sure how? Could this be the order_factor ? The example in the Readme sets this to 'upper'. But to be honest I really can't figure out what it might be doing - the code doesn't seem to check for any particular value like 'upper'? What are the valid values, and what do they each do? This could be documented better (e.g. also added to the "help" option).

# Reorder the landmarks to avoir bias during landmarks set up by humans

Typo "avoir" :) Also the meaning is not clear to me - "[...] during landmarks set up by humans"?

Last thing, feature_names is listed in the Readme as an argument to pts2csv but a CTRL+F of the code yields no results and it's also not part of the function signature?

ybayle commented 5 years ago

Hi @patrikhuber,

Looks good!

Thank you again for your comments and suggestions that help improve a lot our submission and our code. We also learn many python-related things during this process!

I see - but I'm not sure how? Could this be the order_factor ? The example in the Readme sets this to 'upper'. But to be honest I really can't figure out what it might be doing - the code doesn't seem to check for any particular value like 'upper'? What are the valid values, and what do they each do? This could be documented better (e.g. also added to the "help" option).

If the dataset contains lower and upper teeth where only the upper teeth need to be reordered, one can use: Scyland3D.pts2csv("example/", order_factor="upper", order=[36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 37]) or from the command line: python Scyland3D.py -i "example/" -f "upper" -o "36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 37". This is only valid if the keyword upper is present in the file name. Information regarding this feature have been added to the ReadMe and to the code.

Last thing, feature_names is listed in the Readme as an argument to pts2csv but a CTRL+F of the code yields no results and it's also not part of the function signature?

This is fixed and we also added better example in the ReadMe to describe how to use our package from another python script or from the command line.

patrikhuber commented 5 years ago

Hi @ybayle, You're welcome, glad it is helpful.

I understand, thanks for the additional explanations.

I think you forgot to document the feature_names here in the "Args" list: https://github.com/ybayle/Scyland3D/blob/3626b2ad4c339ee2f2483d5a0c06dd94df96f615/Scyland3D.py#L151-L152

Also you should format the .py file with PEP 8, there are a few lines that are too long (and potentially some other things). Any Python IDE should be able to do it, in PyCharm it's "Code => Reformat Code".

Otherwise it looks all good from my side! 👍

ybayle commented 5 years ago

Hi @patrikhuber, Everything should be fine now and I have formatted the code using https://github.com/python/black. Thanks again for your comments!

ybayle commented 5 years ago

Hi @r-barnes: the new version v1.0.6 with all the fixes is now available on pypi https://pypi.org/project/Scyland3D/

patrikhuber commented 5 years ago

@ybayle Just fyi the tooth image doesn't load on the pypi page, at least in my browser. If you have the problem as well, it might be because of the relative URL. I just checked my project and what I did was use the "raw" URL for the image (I probably found that solution on stackoverflow or somewhere): E.g.: https://raw.githubusercontent.com/patrikhuber/eos/gh-pages/images/sfm_shape_3448_mesh.png

ybayle commented 5 years ago

Hi @patrikhuber, The pypi repo now displays correctly the image thanks to your suggested url!

ybayle commented 5 years ago

@Kevin-Mattheus-Moerman Could you please update the version to https://github.com/ybayle/Scyland3D/releases/tag/v1.0.16 in the description?

r-barnes commented 5 years ago

Sorry. Missed the previous message. I'll get to the review shortly.

Kevin-Mattheus-Moerman commented 5 years ago

@whedon set v1.0.16 as version

whedon commented 5 years ago

OK. v1.0.16 is the version.

Kevin-Mattheus-Moerman commented 5 years ago

@patrikhuber thanks for your comments here. Would you like to be be added as an official reviewer so you have a checklist at the top?

Kevin-Mattheus-Moerman commented 5 years ago

@r-barnes thanks for your help with this review. Could you give us an indication as to when you think you can complete the review?

patrikhuber commented 5 years ago

@Kevin-Mattheus-Moerman I don't mind personally. But if there's anything more that would help you (like doing an official checklist), I'd be happy to do it. Should be trivial given I've already completed a pretty much full review and everything is fine from my end. I am on vacation though so the earliest I could do anything is the week of 12 August.

r-barnes commented 5 years ago

I see the following issues:

Other than this, the situation is improved versus the original submission; however, my sense that the software solves too specific a problem in too specific a way remains.