New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: Mapelia and friends: create 3D models from maps #660

Closed
whedon opened this Issue Apr 4, 2018 · 32 comments

Comments

Projects
None yet
5 participants
@whedon
Collaborator

whedon commented Apr 4, 2018

Submitting author: @jordibc (Jordi Burguet-Castell)
Repository: https://github.com/jordibc/mapelia
Version: 1.1.1
Editor: @arfon
Reviewer: @hugoledoux, @ThomasA
Archive: 10.5281/zenodo.1240569

Status

status

Status badge code:

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

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

@hugoledoux & @ThomasA, 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 @arfon know.

Review checklist for @hugoledoux

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (1.1.1)?
  • Authorship: Has the submitting author (@jordibc) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @ThomasA

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (1.1.1)?
  • Authorship: Has the submitting author (@jordibc) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 4, 2018

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @hugoledoux, 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:

  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

This comment has been minimized.

Collaborator

whedon commented Apr 4, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 4, 2018

@hugoledoux

This comment has been minimized.

Collaborator

hugoledoux commented Apr 23, 2018

I am a GIS (geographical information systems) person used to work with datasets, maps, and 3D data.
So our backgrounds are very different.

Maps?!

What is a 'map'? That term in GIS is... vague and can mean:

  1. a paper map
  2. a digital version of one, à la Google Maps

You use it to refer to gridded datasets where the value of each pixel is the elevation or something else. I would be a bit more descriptive here.

Install

I installed it as described, with Python 3.6.5 under macOS. I assume the same would compile under Linux.

I think you should specify that Pillow and numpy are necessary in the install section, I'd have saved a few seconds.

I wonder however if it's possible under Windows?

version

There is no release, thus I can't confirm that v1.1.1 is there... Releases would really help in knowing what is what, now there's only the master branch.

A statement of need

Do the authors clearly state what problems the software is designed to solve and who the target audience is?

That is not entirely clear at this moment. Perhaps a line or two would help? What do you do with the STL files?

Automated tests

These are not there, but I am not sure if they are needed.

pintelia

Why is the use of the textures a separate program? Why not just an option of mapelia?

stl-split

"Split an stl into its north and south sides."
-->
"Split an stl into its north and south HEMISPHERES."

paper.md

Perhaps the references to projections and formats that are in README.txt should be in here too as references?

community guidelines

I don't see any file there.

@arfon

This comment has been minimized.

Member

arfon commented Apr 23, 2018

👋 @ThomasA - just a friendly reminder to try and get to this review sometime soon.

@jordibc

This comment has been minimized.

jordibc commented Apr 23, 2018

Thanks @hugoledoux for the review! I found all the comments appropriate and useful. I've tried to address them:

  • maps: added clarification (both in readme.rst and paper.md)
  • install: added information about the necessary packages in readme.rst
  • version: created a release https://github.com/jordibc/mapelia/releases/tag/v1.1.1
  • a statement of need: added to paper.md an explanation of how it's used in the project A Touch of The Universe
  • automated tests: actually, there were tests, but too hidden in the examples/ directory, so I've added a link to them (tests.py, that can be run directly)
  • pintelia: this one is in fact quite a bit different, it does not generate any kind of reliefs (only colored images superposed to a sphere) and only outputs in ply (because stl does not support colors, even if it is the format commonly used to generate 3D models for printing)
  • stl-split: fixed!
  • paper.md: good point too, references added
  • community guidelines: added contributing.rst with the relevant points

Cheers,
J.

@jordibc

This comment has been minimized.

jordibc commented Apr 23, 2018

@whedon commands

@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 23, 2018

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

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

@jordibc

This comment has been minimized.

jordibc commented Apr 23, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 23, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 23, 2018

@hugoledoux

This comment has been minimized.

Collaborator

hugoledoux commented Apr 24, 2018

@jordibc great that you updated so fast, now all is good as far as I am concerned.

@arfon I updated my review and all the boxes are now ticked

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 24, 2018

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 26, 2018

I have just started reviewing the submission and I will be making remarks here as I go along. I hope that is OK. I may not be able to finish it today...

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 26, 2018

Authorship: It is probably as it should be, but I noticed that there are two authors, Amelia Ortiz-Gil and Jordi Burguet-Castell. On the other hand, only Jordi appears to have made contributions in the code repository.

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 26, 2018

Installation instructions: Just for those interested, the prerequisites can also easily be installed in a new Anaconda environment (here called mapelia-review) using:

conda create -n mapelia-review anaconda=5 pillow numpy

Provided that libgtk is already installed.

@jordibc

This comment has been minimized.

jordibc commented Apr 26, 2018

Just FYI @ThomasA , I'm totally fine with doing the remarks as you go along. I'll wait for my reply until you give me a heads up though, unless you prefer it otherwise.

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 27, 2018

@arfon, how do I make it store the ticks on my check list? If I reload the page they are gone...

@arfon

This comment has been minimized.

Member

arfon commented Apr 28, 2018

@arfon, how do I make it store the ticks on my check list? If I reload the page they are gone...

Please accept the invite at the top of this page, then you should be able to edit the checklist: https://github.com/openjournals/joss-reviews/invitations

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 28, 2018

References: I am not quite sure what the policy is concerning references, but 'paper.bib' contains two references that do not seem to be included in the paper. On the other hand, the references in the paper are not from the paper.bib file. Maybe this is the way it is intended to be, but in that case I do not see any reason to include 'paper.bib' in the repo.

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 28, 2018

A statement of need (documentation): I think that the documentation (in 'readme.rst') could benefit from borrowing a few lines from the paper. The paper starts out with a bit of explanation of what the package is for. I do not find that reflected in the documentation.

@ThomasA

This comment has been minimized.

Collaborator

ThomasA commented Apr 28, 2018

I think this submission meets the requirements overall and I am ready to recommend acceptance, but I would like to hear feedback on my comments on authorship, references, and a statement of need first.

@jordibc

This comment has been minimized.

jordibc commented Apr 29, 2018

Thanks @ThomasA ! Let me try to address those:

  • Authorship: you are right, I did write the code, but Amelia got the idea, saw the directions in which it could grow, made many suggestions, tested it extensively and worked with it to generate the actual 3D-printed models --thus why she is an author too.
  • References: I didn't realize the references were not used, good catch. I fixed them now. As for the ones included in paper.md itself, they provide context that is helpful for the work that is done with the program, but more in the category of details that I didn't consider worth discussing in the paper. Also, I find useful that they are organized in their own sections. But I am not quite sure about the policy there either.
  • A statement of need: added to readme.rst (actually, now it reads the same as the paper).

All the changes are in the master branch. Please let me know if I should I create a tag. I suppose I should at least create one once the review is done, but I'm not sure.

Thanks also for the tip about anaconda!

Cheers,
J.

@jordibc

This comment has been minimized.

jordibc commented Apr 29, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 29, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Collaborator

whedon commented Apr 29, 2018

@arfon arfon added the accepted label Apr 30, 2018

@arfon

This comment has been minimized.

Member

arfon commented Apr 30, 2018

Thanks for your reviews @hugoledoux and @ThomasA!

@jordibc - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

@jordibc

This comment has been minimized.

jordibc commented May 3, 2018

@arfon Done!

It's DOI is 10.5281/zenodo.1240569

(Surprisingly enough, I didn't know about Zenodo... this is a great discovery, thanks!)

@arfon

This comment has been minimized.

Member

arfon commented May 4, 2018

@whedon set 10.5281/zenodo.1240569 as archive

@whedon

This comment has been minimized.

Collaborator

whedon commented May 4, 2018

OK. 10.5281/zenodo.1240569 is the archive.

@arfon

This comment has been minimized.

Member

arfon commented May 4, 2018

@hugoledoux, @ThomasA - many thanks for your reviews here

@jordibc - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00660 ⚡️ 🚀 💥

@arfon arfon closed this May 4, 2018

@whedon

This comment has been minimized.

Collaborator

whedon commented May 4, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](http://joss.theoj.org/papers/10.21105/joss.00660/status.svg)](https://doi.org/10.21105/joss.00660)

This is how it will look in your documentation:

DOI

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment