[REVIEW]: ampscan: A lightweight Python package for shape analysis of prosthetics and orthotics

whedon commented 4 years ago

Submitting author: @JoshuaSteer (Joshua Steer) Repository: https://github.com/abel-research/ampscan Version: v0.3.0 Editor: @Kevin-Mattheus-Moerman Reviewers: @danasolav, @Kevin-Mattheus-Moerman Archive: 10.5281/zenodo.3741495

Status

Status badge code:

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

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

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

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

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 try and complete your review in the next two weeks ✨

Review checklist for @danasolav

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@JoshuaSteer) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @Kevin-Mattheus-Moerman

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@JoshuaSteer) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @danasolav it looks like you're currently assigned to review 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:

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

watching

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

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

@whedon generate pdf

whedon commented 4 years ago

Reference check summary:

OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1109/tnsre.2005.858459 may be missing for title: A method for aligning trans-tibial residual limb shapes so as to identify regions of shape change
- https://doi.org/10.1682/jrrd.2014.10.0272 may be missing for title: Registering a methodology for imaging and analysis of residual-limb shape after transtibial amputation

INVALID DOIs

- None

Kevin-Mattheus-Moerman commented 4 years ago

@whedon add @Kevin-Mattheus-Moerman as reviewer

whedon commented 4 years ago

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

whedon commented 4 years ago

OK, @Kevin-Mattheus-Moerman is now a reviewer

Kevin-Mattheus-Moerman commented 4 years ago

@whedon commands

whedon commented 4 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check repository

whedon commented 4 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.09 s (566.2 files/s, 69726.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          22            605           1508           2160
SVG                              3              5              2            507
Markdown                         4             60              0            144
Jupyter Notebook                 3              0            688            104
reStructuredText                12             41             71             78
TeX                              1              5              0             45
Cython                           1              4              5             42
DOS Batch                        1              8              1             27
YAML                             2              1              0             26
make                             1              4              6             10
-------------------------------------------------------------------------------
SUM:                            50            733           2281           3143
-------------------------------------------------------------------------------

Statistical information for the repository '2060' was gathered on 2020/02/01.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Jack Parsons                     1             1              1            0.00
Joshua Steer                   116          6295           4305            6.04
JoshuaSteer                     52          7163           6401            7.73
Oliver Stocks                   84         73354          73227           83.58
Omar Animashaun                 17          1139            700            1.05
jack-parsons                    66          1345            777            1.21
ojs1g14                         88           535            138            0.38

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Joshua Steer               3707           58.9          0.8               13.27
jack-parsons                393           29.2          3.3               23.16
ojs1g14                     173           32.3         16.0               48.55

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer

[x] Authorship / contributions from the above report it looks like Omar Animashaun contributed but it is not listed as an author or acknowledged in the paper. Can you comment on this? If they are not included as author (e.g. if their contributions were minor enough), perhaps you can still acknowledge other contributors like these in the paper?
[x] References some hyperlinks are provided for other packages (i.e. in ...such as mayavi for scientific data visualisation, open3d for alignment and registration techniques, and gibbon for...). Can you add references to articles here where possible? E.g. GIBBON should be cited with this paper: https://joss.theoj.org/papers/10.21105/joss.00506
[x] Can you add the missing DOI's see below (check all our bot may have missed other missing ones) You can call @whedon generate pdf and @whedon check references here yourself too if you want.

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check references

whedon commented 4 years ago

Reference check summary:

OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1109/tnsre.2005.858459 may be missing for title: A method for aligning trans-tibial residual limb shapes so as to identify regions of shape change
- https://doi.org/10.1682/jrrd.2014.10.0272 may be missing for title: Registering a methodology for imaging and analysis of residual-limb shape after transtibial amputation

INVALID DOIs

- None

Kevin-Mattheus-Moerman commented 4 years ago

@danasolav :wave: we've started reviewing here. Thanks again for your help!

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer See my earlier comments and these additional comments.

[x] Can you work on adding clear contributing guidelines? (one of mine for example is here: https://github.com/gibbonCode/GIBBON/blob/master/CONTRIBUTING.md you can link to this in the readme). But you can be as brief/extensive as you want. Perhaps you can link to Python style standards. You can search around a bit for what is commonly used. If you intend to have this community grow I would consider adding a code of conduct as well.
[x] In relation to testing. I ran the 3 examples as Jupyter notebooks. This is convenient and works but it does not say this anywhere (unless I missed it). It seems the documentation site just renders these jupyter notebooks. So a section with testing instructions would be good.
[x] Can you add automated tests (and instructions for this)? E.g. a more advanced (non-Jupyter notebook) implementation.
[x] Check for the typos vertrx, and convienience in the fundamentals demo.
[x] It looks like you migrated to GitHub from GitLab. The documentation still refers to GitLab though so perhaps fix that.
[x] Recommendation: Volume computation does not require slicing. And by using slicing it becomes less accurate than it can be (as shown in the coarse/fine comparison). Have you thought about exact volume computation based on Gauss divergence theorem (here is a MATLAB implementation https://github.com/gibbonCode/GIBBON/blob/master/lib/triSurfVolume.m, note for quads you can first convert to triangular faces by slashing the quads). You probably need to close over the top of the scan though (although that should not be too hard).
[x] Recommendation: I did not check your smoothing implementation but saw statements on shape changes induced by it (which is common especially for standard Laplacian based smoothing). Have you seen this simple smooth method called HC smoothing it helps to reduce "shrinkage" effects.

JoshuaSteer commented 4 years ago

@Kevin-Mattheus-Moerman Thanks for all the comments, all great suggestions to improve ampscan.

Quick Q on testing, we've got our whole package linked up to travis CI (https://travis-ci.org/abel-research/ampscan), so I guess you are referring to enabling for someone to run these tests locally on install as well?

Thanks!

JoshuaSteer commented 4 years ago

@whedon check references

whedon commented 4 years ago

Reference check summary:

OK DOIs

- 10.1109/tnsre.2005.858459 is OK
- 10.1682/jrrd.2014.10.0272 is OK
- 10.1177/0309364619883197 is OK
- 10.1007/s10237-019-01195-5 is OK
- 10.21105/joss.00506 is OK
- 10.1109/MCSE.2011.35 is OK
- 10.2312/LocalChapterEvents/ItalChap/ItalianChapConf2008/129-136 is OK

MISSING DOIs

- None

INVALID DOIs

- None

Kevin-Mattheus-Moerman commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer

Quick Q on testing, we've got our whole package linked up to travis CI (https://travis-ci.org/abel-research/ampscan), so I guess you are referring to enabling for someone to run these tests locally on install as well?

Apologies. Missed that. That is fine. I ticked the automated tests box.

JoshuaSteer commented 4 years ago

@Kevin-Mattheus-Moerman

@JoshuaSteer

Quick Q on testing, we've got our whole package linked up to travis CI (https://travis-ci.org/abel-research/ampscan), so I guess you are referring to enabling for someone to run these tests locally on install as well?

Apologies. Missed that. That is fine. I ticked the automated tests box.

No worries, it wasn't immediately obvious on the docs/repo that we had an automated testing package, so I've edited that now.

Contribution guidelines, code of conduct, typos, acknowledged contributors, DOIs have all been changed as requested.

Great recommendations! We had planned to add in a HC smoothing algorithm (as you say, normal issues of volume loss with standard laplacian) and a 'true' volume calculation, so now is as good a time as any to include them.

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer thanks for working on my suggestions. In relation to testing perhaps instruct users to set up Jupyter and run the notebooks. Is this how you intend users to run the tutorials? Is this written somewhere already?

Some minor bits:

[x] Check typos in the README, see these: Licesnse, founde, and here here
[x] I suggest linking to your contributing guidelines in the README
[x] A super minor nitpicky point. Perhaps capitalize laplacian throughout documentation (I see laplacian in the fundamentals demo).
[x] Did you see the authorship/contributor acknowledgement point I made above ☝️

JoshuaSteer commented 4 years ago

@Kevin-Mattheus-Moerman Good idea about the Jupyter notebooks for testing. I'll add in a section which describes to process to the user.

I've added in the HC smoothing algorithm, with a test on Travis and it's included in the fundamentals notebook where you can compare the shrinkage relative to the Laplacian.

Omar has been acknowledged within the paper now. Felt this was more appropriate than as an author considering relative contribution.

Thanks for picking up on those details. I'll let you know when they're done!

danasolav commented 4 years ago

@JoshuaSteer, have you tried a fresh installation of the package on Windows 10? I have been struggling in the last two days to get it running with no success. I keep having issues with dependencies. For example, it seems like installing ampscan already includes installation of vtk, but in the readme you ask to pre-install vtk, so I got an error when it unsuccessfully tried to uninstall the existing vtk. After fixing that I still had a lot of other issues. Any suggestions besides installing Linux?

JoshuaSteer commented 4 years ago

@danasolav I work using Windows 10, and my build works fine at the moment. However, the recent versions of PyQt and VTK appear to be throwing some installation issues, so it may be best to adjust the installation instructions to just use pip rather than conda. I'll run some fresh installs on my machine to see if any issues. Can you send me some screenshots?

JoshuaSteer commented 4 years ago

@danasolav I've run some fresh installs on Windows 10 without problems, if you have a traceback of the error that'd be great

danasolav commented 4 years ago

@JoshuaSteer Thanks. I'll try again today and send you some screenshots and traceback. as far as I remember the errors where related to vtk, PyQt5, and PyPDF2. Can you share which environment you're using and if used conda or pip to install the packages this time?

danasolav commented 4 years ago

@JoshuaSteer. Good news! I was able to install and run the GUI. I will send my comments soon.

danasolav commented 4 years ago

@JoshuaSteer, here are my comments/questions (mostly referring to the GUI):

[x] Could you add some general information in the readme about what the toolbox is useful for. For example- why would aligning to shapes could be used for? What are the analysis capabilities?
[x] Could you add instructions and explanations for using the GUI? For example, in Align: What is Static and Moving, how do you use the different options (Centre, ICP, translation/rotation) and in which order? I noticed for example that trying to use Centre or translations/rotations after ICP doesn’t work.
[x] In my experience, ICP can be very sensitive to the initial position and orientation of the two shapes. Is that what the rotations and translations are for? to bring the shapes close together before ICP?
[x] Could you add a coordinate system to the visualization in Align? Without it it’s much harder to use the translations/rotations and there is also no scale for the size of the limb.
[x] Could you add a colorbar to the Registration results and an explanation in the readme as to what is the visualized measure?
[x] Could you add an example file that can be loaded in FE Analysis? Without it, it’s not clear what Analyse in the GUI does. I’m also confused because the documentation says that the analyse module does the slicing and volume calculation but in the GUI “Analyse” requires an FEA object.
[x] I couldn't find an explanation anywhere as to what Kinetic Measurement is?
[x] I tried using Generate Measurements after picking the mid patella point but it caused the GUI to crash and close. (FileNotFoundError: [Errno 2] No such file or directory: 'res\Measurements Template.pdf'). I see this file in the res folder so I’m not sure why it fails to obtain it. Anyway, it looks like the template file doesn't contain volume. So is there a way to obtain volume measurement using the GUI?
[x] In Registration, why are the vertices morphed onto the other shape? Is it for having a direct vertex-to-vertex displacement value? There are also ways to compute it when the vertices do not correspond.
[x] I ran some tests using different STLs which are all originated from the same scan but in different rotations/ different mesh density/ top edge trimmed differently. I found that sometimes the ICP algorithm fails if I run it without making the two shapes very close first. These are the STLs I used for testing, in case you want to try them: https://www.dropbox.com/sh/6ubuj01y2a6cp6a/AADfI-ODmkXbR5Qf10X7v1TEa?dl=0

JoshuaSteer commented 4 years ago

@danasolav Thanks for all the suggestions. A few things we've got in the pipeline to fix. There's a couple of legacy elements of the GUI which need removing as well which you picked up on.

I'll start working through them and let you know when they're updated

Big thanks for providing your STL files for testing - very helpful!

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer let us know when you are ready with those edits and for us to resume the review process.

JoshuaSteer commented 4 years ago

Thanks @Kevin-Mattheus-Moerman

@danasolav exposed some good bugs to tackle in the GUI. I've done a fairly sizeable restructure based on this. Should have edits finished in next day or so

JoshuaSteer commented 4 years ago

Thanks for all your great comments. It's really helped improve and focus the package:

@Kevin-Mattheus-Moerman all your following have now been sorted

Check typos in the README, see these: Licesnse, founde, and here here
I suggest linking to your contributing guidelines in the README
A super minor nitpicky point. Perhaps capitalize laplacian throughout documentation (I see laplacian in the fundamentals demo).

@danasolav

Could you add some general information in the readme about what the toolbox is useful for. For example- why would aligning to shapes could be used for? What are the analysis capabilities? I've added a whole section to the readme which gives a high level overview of the toolbox and the capabilities of the individual modules
Could you add instructions and explanations for using the GUI? For example, in Align: What is Static and Moving, how do you use the different options (Centre, ICP, translation/rotation) and in which order? I noticed for example that trying to use Centre or translations/rotations after ICP doesn’t work. _I've added a new guide on the readthedocs page_
In my experience, ICP can be very sensitive to the initial position and orientation of the two shapes. Is that what the rotations and translations are for? to bring the shapes close together before ICP? Yes, and in our experience as we are performing ICP on dissimilar shapes, you may want to refine the automated alignment based upon your protocol
Could you add a coordinate system to the visualization in Align? Without it it’s much harder to use the translations/rotations and there is also no scale for the size of the limb. Great suggestion,Triad now added to the GUI
Could you add a colorbar to the Registration results and an explanation in the readme as to what is the visualized measure? Colourbar now appears in the GUI
Could you add an example file that can be loaded in FE Analysis? Without it, it’s not clear what Analyse in the GUI does. I’m also confused because the documentation says that the analyse module does the slicing and volume calculation but in the GUI “Analyse” requires an FEA object.
I couldn't find an explanation anywhere as to what Kinetic Measurement is? Both of these should not have been here, they are legacy features from before we streamlined the software. Thanks for pointing them out as they've now been removed
I tried using Generate Measurements after picking the mid patella point but it caused the GUI to crash and close. (FileNotFoundError: [Errno 2] No such file or directory: 'res\Measurements Template.pdf'). I see this file in the res folder so I’m not sure why it fails to obtain it. Anyway, it looks like the template file doesn't contain volume. So is there a way to obtain volume measurement using the GUI? It fails if the GUI wasn't booted from the correct folder. It should now always work as I've improved the file path manger for the res file
In Registration, why are the vertices morphed onto the other shape? Is it for having a direct vertex-to-vertex displacement value? There are also ways to compute it when the vertices do not correspond. Which methods are you referring to specifically, for our purposes we find registration works well - often we compare coarse and fine meshes which require a point-to-plane style mesh
I ran some tests using different STLs which are all originated from the same scan but in different rotations/ different mesh density/ top edge trimmed differently. I found that sometimes the ICP algorithm fails if I run it without making the two shapes very close first. These are the STLs I used for testing, in case you want to try them: https://www.dropbox.com/sh/6ubuj01y2a6cp6a/AADfI-ODmkXbR5Qf10X7v1TEa?dl=0 Thanks for these, I didn't have these problems when I ran the algorithms on the various combinations, do you have an output of the error message?

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

danasolav commented 4 years ago

Sorry for the delay. @JoshuaSteer- thanks for answering my comments. I've updated my checklist, and the paper is ready for publication as far as I'm concerned.

JoshuaSteer commented 4 years ago

Thanks @danasolav - no worries about the delay. Thank you for taking the time to give all your great comments, they've really improved the software!

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer thanks for making those changes. I think you addressed all my main points. I was wondering if you considered my suggesting of using the Gaussian divergence theorem for volume computation (which provides the exact volume and does not rely on slicing). This is not a requirement, you can argue you are happy with the slicing based method for now, I just wanted to know if you worked on this. I've posted a related issue here: https://github.com/abel-research/ampscan/issues/53 in case you could consider this as a future development.

JoshuaSteer commented 4 years ago

Thanks @Kevin-Mattheus-Moerman

Great news that all your main comments have been addressed. I'd quite like to the volume computation in as it would finish off the package quite nicely. I'll hopefully get it done by today and then we can get this published. Thanks for all of your great comments!

JoshuaSteer commented 4 years ago

@JoshuaSteer thanks for making those changes. I think you addressed all my main points. I was wondering if you considered my suggesting of using the Gaussian divergence theorem for volume computation (which provides the exact volume and does not rely on slicing). This is not a requirement, you can argue you are happy with the slicing based method for now, I just wanted to know if you worked on this. I've posted a related issue here: abel-research/ampscan#53 in case you could consider this as a future development.

@Kevin-Mattheus-Moerman @danasolav

This has now been done, thanks for the link to your algorithm as a reference, it worked great. I've updated the docs online, where you can see the result of the simple hole filling algorithm I wrote and how it changes the volume computation.

https://ampscan.readthedocs.io/en/latest/examples/analysis.html#Volume,-Cross-section-area-and-Widths

Hopefully that's now everything and we're ready to move to publishing!

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer those changes look good. Thanks for implementing that volume computation. I have not more issues remaining. I'm happy (in my role as reviewer) to recommend this for acceptance in JOSS.

Kevin-Mattheus-Moerman commented 4 years ago

@danasolav thanks again for reviewing this work. You've used tickboxes in this comment https://github.com/openjournals/joss-reviews/issues/2060#issuecomment-590065452 here. Would you be able to tick those boxes as well for completeness? Thanks

Kevin-Mattheus-Moerman commented 4 years ago

@JoshuaSteer your work is about to be processed for acceptance in JOSS.

[ ] Please thoroughly review your paper including the author names and affiliations one last time.
[ ] Once you've read and updated your paper please post an archived version of your software on Zenodo. The Zenodo archived version's meta data, such as the title and author list, should match those of your paper! (these instructions might be helpful: https://guides.github.com/activities/citable-code/)
[ ] Once you've archived a version there please report back the DOI of the archived version here.
[ ] Can you confirm what the version tag is for the archived version of your software.

Kevin-Mattheus-Moerman commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check references

whedon commented 4 years ago

Reference check summary:

OK DOIs

- 10.1109/tnsre.2005.858459 is OK
- 10.1682/jrrd.2014.10.0272 is OK
- 10.1177/0309364619883197 is OK
- 10.1007/s10237-019-01195-5 is OK
- 10.21105/joss.00506 is OK
- 10.1109/MCSE.2011.35 is OK
- 10.2312/LocalChapterEvents/ItalChap/ItalianChapConf2008/129-136 is OK

MISSING DOIs

- None

INVALID DOIs

- None

JoshuaSteer commented 4 years ago

@JoshuaSteer those changes look good. Thanks for implementing that volume computation. I have not more issues remaining. I'm happy (in my role as reviewer) to recommend this for acceptance in JOSS.

@Kevin-Mattheus-Moerman Excellent news! Thank you for all of yours and @danasolav work on this!

JoshuaSteer commented 4 years ago

@JoshuaSteer your work is about to be processed for acceptance in JOSS.

[ ] Please thoroughly review your paper including the author names and affiliations one last time.

[ ] Once you've read and updated your paper please post an archived version of your software on Zenodo. The Zenodo archived version's meta data, such as the title and author list, should match those of your paper! (these instructions might be helpful: https://guides.github.com/activities/citable-code/)

[ ] Once you've archived a version there please report back the DOI of the archived version here.

[ ] Can you confirm what the version tag is for the archived version of your software.

@Kevin-Mattheus-Moerman

@JoshuaSteer thanks for working on my suggestions. In relation to testing perhaps instruct users to set up Jupyter and run the notebooks. Is this how you intend users to run the tutorials? Is this written somewhere already?

Some minor bits:

[x] Check typos in the README, see these: Licesnse, founde, and here here

[x] I suggest linking to your contributing guidelines in the README

[x] A super minor nitpicky point. Perhaps capitalize laplacian throughout documentation (I see laplacian in the fundamentals demo).

[x] Did you see the authorship/contributor acknowledgement point I made above ☝️

@JoshuaSteer your work is about to be processed for acceptance in JOSS.

[x] Please thoroughly review your paper including the author names and affiliations one last time.

[x] Once you've read and updated your paper please post an archived version of your software on Zenodo. The Zenodo archived version's meta data, such as the title and author list, should match those of your paper! (these instructions might be helpful: https://guides.github.com/activities/citable-code/)

[x] Once you've archived a version there please report back the DOI of the archived version here.

[x] Can you confirm what the version tag is for the archived version of your software.

@Kevin-Mattheus-Moerman

DOI: 10.5281/zenodo.3741495 Version: v0.3.0

arfon commented 4 years ago

@whedon set 10.5281/zenodo.3741495 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.3741495 is the archive.

openjournals / joss-reviews