-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[REVIEW]: morphMan: Automated manipulation of vascular geometries #1065
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @brainstorm, it looks like you're currently assigned as the reviewer for this paper 🎉. ⭐ Important ⭐ 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:
For a list of things I can do to help you, just type:
|
|
@aslakbergersen No references in |
@brainstorm thank you for agreeing to review morphMan. I'm not sure I understand what you are asking. There are 13 references in the paper.md, but the bibliography (paper.bib) is located here. I hope that answered your question. |
My bad, I was reading the rendered version and I saw that section empty, but they are indeed in the .bib as they should, ticking off that box ;) |
Hi @aslakbergersen I can see there are still some open issues and unchecked items on the reviewer's lists so I wanted to check if everything was in order. |
Hi @trallard, thank you for checking in. After comments from both reviewers, we have just changed the installation method, meeting the comments in KVSlab/morphMan#14. @brainstorm and @rlizzo is there anything else we should address? |
I can see that both reviewers left the following point of the checklist unchecked:
@brainstorm @rlizzo can you advise on this? I saw there is a read the docs website but I am unsure if you both deemed that more work would need to be done at an API level |
@aslakbergersen I advised the authors to publish their package under conda-forge for easy installation/access instead of creating their own channel on Anaconda. @aslakbergersen Do you reckon you can put it there? Also, the tests seem to be passing on appveyor, but I cannot make them pass locally after doing the equivalent of:
On my system under morphman conda virtual environment. There should be a mini-section on the readme on how to make the tests run locally.
|
@brainstorm We would prefer to have morphman on conda-forge, however, since we rely on VMTK, which is not on conda-forge (yet), we instead created your own channel, for now, cf. conda-forge dependencies. @rlizzo You mentioned that vmtk eventually will be moved over to conda-forge, how is that work going, and is there something we could contribute with? @brainstorm, regarding running the test locally. Thank you for pointing this out, because this is a critical bug. In vmtk version 1.4 there is a python3 bug in how the centerlines are created, as a workaround, we modify the vmtk installation in both TravisCI and AppVayor, cf. this line for travis and this line for AppVayor. The problem is fixed in master for vmtk, so while we wait for a updated vmtk package to be relseased in conda we have added a warning to the installation instructions with a workaround for the problem. |
|
@rlizzo I know we all have a lot on our todo-lists now before Christmas holiday break, but I'm wondering if you have any additional comments, or if it would be possible to wrap this up before the Holidays? |
@rlizzo ping, I hope you had a nice holiday season I was wondering if there are still outstanding actions to proceed with the review? |
It seems that @rlizzo has gone under the radar, @brainstorm I know you gave some sort of ok before but I just want to check if you are happy recommending this package for acceptance or if any changes should be made (there is still an unchecked item on your list) |
I just gave it another look/try and I would advise against accepting the installation facet of this paper. I still would recommend to have vmtk packaged in conda-forge (as a dependency), as @rlizzo diligently pointed out in vmtk/conda-recipes#2 as well as in KVSlab/morphMan#14. From a functional perspective, I would like to have easier means to download test data locally in order to evaluate the software (had to fish it out from the testsuite). Something like a simple Other than those two small details, I would totally endorse this nice piece of software. |
Thank you for testing the software @brainstorm, it is much appreciated.
I agree that there should be an additional script to ease playing around with the test data. To take it one step further, I took the command line examples from the Tutorial and rewrote them as python demos as well, see KVSlab/morphMan#20. I also added a script which downloads the test data. Please also note that there are links for downloading the test data in the tutorials.
Just a clarification before starting to move the installation to conda-forge: Do I understand you correctly if you are asking us to add a vmtk recipe to conda-forge, and then move morphMan into conda-forge when it is accepted? If that is the case, I would like to confer with the main developers of vmtk first, like @rlizzo. Or, do you propose that we add vmtk as an outside dependency, although disadvised. |
That's fantastic Aslak, thanks for putting it together as an script! And yes, I would advise to add When developers/PhDs/postdocs/people move on with other duties, if the dependencies are not maintained as well as the main package, bitrot happens faster, that's why I would like to insist on that point. |
@brainstorm, I agree that moving the project into conda-forge is the better option. We have been working on adding Adding |
Hi Guys, I'm sorry I've been away. This has been a horribly busy start of the year that kept me off of github for a bit. If it's worth anything, the installation methods that I had previously commented on seem to be taken care of by @aslakbergersen's work on the conda-forge recipes. I've cleared up the rest of the checkboxes from my end. I'm terribly sorry to hold up the process. here. The package looks good from my end. |
@aslakbergersen is there any update with regards to the status of this? Have all the reviewers concerns been addressed? |
@trallard thank you for the follow-up. We are still working on moving |
|
@whedon accept |
|
PDF failed to compile for issue #1065 with the following error: % Total % Received % Xferd Average Speed Time Time Time Current 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 |
@whedon accept |
|
Sometimes @whedon times out generating these PDFs... |
|
PDF failed to compile for issue #1065 with the following error: /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in |
|
@whedon accept |
|
@whedon generate pdf |
|
@whedon accept deposit=true |
|
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? notify your editorial technical team... |
@brainstorm, @rlizzo - many thanks for your reviews here and to @trallard for editing this submission ✨ @aslakbergersen @hkjeldsberg - your paper is now accepted into JOSS ⚡🚀💥 |
👋 Hey @arfon... Letting you know, |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @aslakbergersen (Aslak Bergersen)
Repository: https://github.com/KVSlab/morphMan
Version: v0.2
Editor: @trallard
Reviewer: @brainstorm, @rlizzo
Archive: 10.5281/zenodo.2591725
Status
Status badge code:
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
@brainstorm & @rlizzo, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @trallard know.
✨ Please try and complete your review in the next two weeks ✨
Review checklist for @brainstorm
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?Review checklist for @rlizzo
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?The text was updated successfully, but these errors were encountered: