search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
708 stars 37 forks source link

[REVIEW]: splot: visual analytics for spatial statistics #1882

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @slumnitz (Stefanie Lumnitz) Repository: https://github.com/pysal/splot Version: v1.1.1 Editor: @leouieda Reviewer: @ResidentMario, @martinfleis Archive: 10.5281/zenodo.3724199

Status

status

Status badge code:

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

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

@ResidentMario & @martinfleis, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @leouieda know.

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

Review checklist for @ResidentMario

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @martinfleis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ResidentMario, @martinfleis 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:

  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

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 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

leouieda commented 4 years ago

👋 @slumnitz @ResidentMario, @martinfleis this is the review thread for the paper. All of our communications will happen here from now on.

Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#1882 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me here (@leouieda) or email me privately if you have any questions/concerns.

martinfleis commented 4 years ago

Hi,

I would like to close up my review. Majority of splot's feature is currently focused on visualising the data from various PySAL sub-modules, which is extremely helpful in understanding their outputs. However, splot goes further than that (and has plans to do even more). A prime example is Value-by-alpha plotting feature. The default visual style of splot is very polished, and I would also say that there is no need to touch the defaults and plots can go directly to publications. That is not very common for scientific software.

During my review, I have opened a couple of issues (all linked above). Most of them are just recommendations, rather than critical issues. Before I tick Installation box, I would like to see pysal/splot#88 fixed as there are currently two versions of instructions, with those in the documentation failing. The second issue I would like to see resolved to tick the final box is pysal/splot#92, but that seems to be just a syntax issue.

To sum it up, splot is a valuable part of Python geospatial world and I will be watching closely its future development. My last recommendation is to have a detailed discussion with geoplot (already started in pysal/splot#21) to agree on the scope of both projects and avoid duplication of effort in the future.

ResidentMario commented 4 years ago

I'm done with my review. I don't have any hard blocking issues to add to those opened by @martinfleis.

But I will say that I have some qualms with the uneven documentation in splot. For example, one of the three tutorial notebooks, titled Exploratory Analysis of Spatial Data, mentions the following statistical concepts without giving much explanation or linking the user to a reference on what these things are: Moran's I, local Moran, Donatns [sic?] variable, bivariate Moran statistic. Furthermore, the tutorials only cover a small subset of the library functions: there are many plotters documented at some length in the API Reference which do not appear in the any of the tutorials.

I do not think this is a blocking issue because the documentation is still within the acceptable range for JOSS (all of the relevant methods are covered by the API Reference, which doesn't leave any fields unexplained and is generally very well-packed with example images), but I feel that it's a definite area for improvement for the package as a whole.

I have left more general feedback about documentation with the broader PySAL team in the PySAL Gitter chat.

leouieda commented 4 years ago

Hi @martinfleis @ResidentMario thank you for your reviews! :tada:

@ResidentMario that is a good point about the tutorial coverage and I agree that it's not a requirement for JOSS to have full coverage of tutorials. However, the citation/linking of statistical concepts is highly encouraged as I see scientific software documentation (particularly for JOSS) as a major part of the scholarship. @slumnitz could you please go through the tutorials and add references as you see necessary?

@martinfleis @slumnitz please let me know when the install and figure caption issues are resolved.

slumnitz commented 4 years ago

@martinfleis @ResidentMario @leouieda thank you for your thorough review, comments and great suggestions! I will implement them in the next couple of weeks or as soon as I can and let you know when everything is done.

danielskatz commented 4 years ago

@slumnitz - any news on this?

slumnitz commented 4 years ago

@slumnitz - any news on this?

@danielskatz I will start working on it slowly again now. It got a bit delayed due to snowmageddon and a couple of electricity outages here in BC, Canada, which are expected to ease in the next wo weeks.

leouieda commented 4 years ago

@slumnitz thank you for the update :+1: Please keep us informed of your progress.

slumnitz commented 4 years ago

@martinfleis @ResidentMario and @leouieda thank you for your review and comments. The new splot release includes solutions to the major documentation issues outlined. Currently as you noticed, splot tutorials require previous knowledge in spatial statistical analysis and are not aimed to teach statistical concepts.

Do you think a new tutorial like a beginners guide to spatial statistical analysis in splot would be suitable to fill this gap, or would you recommend referencing great blogposts, statistical literature is enough?

martinfleis commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

martinfleis commented 4 years ago

@slumnitz the figure captions formatting is still strange. JOSS has its own style for captions, see the last published paper. Your figures seems to be encapsulated in links, which are not working anyway, causing captions not to render.

martinfleis commented 4 years ago

@slumnitz to answer your question, I think good links are enough.

leouieda commented 4 years ago

@slumnitz thank you keeping us in the loop. As @martinfleis said, the figures weren't rendering properly. I submitted a PR fixing this.

Also, we now have a compilation service that you can use to preview the paper (even on branches before merging PRs): https://whedon.theoj.org

@ResidentMario I see you still have the "Installation" review item unchecked. Could you please see if this has been addressed in the new version of splot?

@martinfleis you have the "Quality of writing" item unchecked. Has this been addressed in the updated paper?

martinfleis commented 4 years ago

@leouieda I kept it unchecked due to figures formatting issue. Your PR resolves it, but note that you made a PR to your forked repo not the original repo.

I'll take it as resolved and finish my review.

ResidentMario commented 4 years ago

I had the installation mark unchecked due to splot#88, but that's now been fixed so I went ahead and checked the box.

The new docs look a lot better!

That's everything from me.

leouieda commented 4 years ago

Your PR resolves it, but note that you made a PR to your forked repo not the original repo.

Well, that was embarrassing :) Resolved now with pysal/splot#99

@martinfleis and @ResidentMario thank you for your reviews!

@slumnitz I'll go over the submission one more time to make any editorial suggestions that might be needed. After that, I'm happy to move forward with acceptance of your submission to JOSS :tada:

leouieda commented 4 years ago

@whedon check references

ljwolf commented 4 years ago

The rerender of the paper after merging pysal/splot#99 is here and should conform to the requests, so let me or @slumnitz know if anything else is required.

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.

ljwolf commented 4 years ago

Thanks @arfon, I understand this is a challenging time for everyone. I believe the submission is complete; @leouieda's suggestions were the last ones remaining, and have been addressed.

arfon commented 4 years ago

@whedon accept

whedon commented 4 years ago

No archive DOI set. Exiting...

arfon commented 4 years ago

@ljwolf - I'm not sure of @leouieda's availability to handle the final steps here so...

At this point could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:

I can then move forward with accepting the submission.

ljwolf commented 4 years ago

Will do. Thanks!

slumnitz commented 4 years ago

@ljwolf thanks for taking this on. I finally have a stable wifi connection again here in Italy. I will take this on tonight. Would you be able to review as soon as I made the PR's?

slumnitz commented 4 years ago

@arfon thank you for helping. Splot has a new release v.1.1.3 on Zenodo

DOI: 10.5281/zenodo.3724199

I am not sure how I can update this thread. Maybe you could help? Thank you.

slumnitz commented 4 years ago

@whedon commands

whedon commented 4 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

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

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

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

# 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
arfon commented 4 years ago

@whedon set 10.5281/zenodo.3724199 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.3724199 is the archive.

arfon commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

arfon commented 4 years ago

@whedon accept

whedon commented 4 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 4 years ago

PDF failed to compile for issue #1882 with the following error:

E, [2020-03-23 14:07:26#418] ERROR -- : Failed to parse BibTeX on value "--140" (NAME) [#, "@", #, {:author=>["Robert E Roth and Andrew W Woodruff and Zachary F Johnson"], :title=>["Value-by-alpha maps: {An} alternative to the cartogram"], :journal=>["The Cartographic Journal"], :year=>["2010"], :volume=>"47", :issue=>"2", :pages=>"130"}] bibtex.y:138:in on_error': Failed to parse BibTeX on value "--140" (NAME) [#<BibTeX::Bibliography data=[12]>, "@", #<BibTeX::Entry >, {:author=>["Robert E Roth and Andrew W Woodruff and Zachary F Johnson"], :title=>["Value-by-alpha maps: {An} alternative to the cartogram"], :journal=>["The Cartographic Journal"], :year=>["2010"], :volume=>"47", :issue=>"2", :pages=>"130"}] (BibTeX::ParseError) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/racc/parser.rb:259:in_racc_do_parse_c' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/racc/parser.rb:259:in do_parse' from bibtex.y:111:inparse' from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.1.2/lib/bibtex/bibliography.rb:67:in parse' from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.1.2/lib/bibtex/bibliography.rb:50:inopen' from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.1.2/lib/bibtex/utilities.rb:25:in open' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/lib/whedon/bibtex_parser.rb:36:ingenerate_citations' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/lib/whedon/compilers.rb:245:in crossref_from_markdown' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/lib/whedon/compilers.rb:21:ingenerate_crossref' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/lib/whedon/processor.rb:95:in compile' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/bin/whedon:79:incompile' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-45a043c4bfc2/bin/whedon:116:in <top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in `

'

arfon commented 4 years ago

@slumnitz - this PR should fix the BibTeX issue here: https://github.com/pysal/splot/pull/104

arfon commented 4 years ago

@whedon accept

whedon commented 4 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1007/978-3-319-50590-9_17 is OK
- 10.1016/j.apgeog.2013.09.012 is OK
- 10.1007/s10109-009-0086-8 is OK
- 10.1007/978-3-319-59511-5_2 is OK
- 10.5281/zenodo.3333010 is OK
- 10.5281/zenodo.3475569 is OK

MISSING DOIs

- https://doi.org/10.1080/01944360802146329 may be missing for title: The Ghost Map: The Story of London’s Most Terrifying Epidemic–and How It Changed Science, Cities, and the Modern World
- https://doi.org/10.1111/j.1435-5597.1988.tb01155.x may be missing for title: Do Spatial Effects Really Matter in Regression Analysis?

INVALID DOIs

- 10.1179/000870409X124887534553372 is INVALID
whedon commented 4 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1388

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1388, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 4 years ago

@slumnitz - could you take a look at the DOI summary above to check to see if the DOIs under MISSING are correct recommendations and also check the INVALID DOI?

slumnitz commented 4 years ago

@arfon all missing doi's are added and the incorrect one was fixed.

arfon commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1080/01944360802146329 is OK
- 10.1007/978-3-319-50590-9_17 is OK
- 10.1016/j.apgeog.2013.09.012 is OK
- 10.1111/j.1435-5597.1988.tb01155.x is OK
- 10.1007/s10109-009-0086-8 is OK
- 10.1007/978-3-319-59511-5_2 is OK
- 10.5281/zenodo.3333010 is OK
- 10.5281/zenodo.3475569 is OK
- 10.1179/000870409X12488753453372 is OK

MISSING DOIs

- None

INVALID DOIs

- None
arfon commented 4 years ago

@whedon accept deposit=true

whedon commented 4 years ago 
Doing it live! Attempting automated processing of paper acceptance...