openjournals / joss-reviews

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

[REVIEW]: ComplexRegions.jl: A Julia package for regions in the complex plane #1811

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @tobydriscoll (Tobin Driscoll) Repository: https://github.com/complexvariables/ComplexRegions.jl Version: v0.1.1 Editor: @drvinceknight Reviewer: @dlfivefifty, @dpsanders Archive: 10.5281/zenodo.3548866

Status

status

Status badge code:

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

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

@dlfivefifty & @dpsanders, 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 @drvinceknight know.

Please try and complete your review in the next two weeks

Review checklist for @dlfivefifty

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @dpsanders

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. @dlfivefifty, @dpsanders 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:

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

I think my review is essentially done. The main thing is referencing related work of IntervalSets.jl and DomainSets.jl, see

https://github.com/complexvariables/ComplexRegions.jl/issues/1

drvinceknight commented 4 years ago

Thank you @dlfivefifty :+1: 💪

dpsanders commented 4 years ago

@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:

dpsanders commented 4 years ago

My main review is now complete.

There is basically nothing written down about how to contribute to the package, but I think it's understood this is done initially via issues, which seems fine to me.

I opened a couple of issues in the repo with suggestions for improvements.

drvinceknight commented 4 years ago

Thank @dpsanders and @dlfivefifty, @tobydriscoll if you could confirm when you've addressed the comments that would be great.

tobydriscoll commented 4 years ago

@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:

dpsanders commented 4 years ago

There seems to be a problem with unicode characters in the Julia source code. @drvinceknight what can be done about that?

tobydriscoll commented 4 years ago

I'm not married to using unicode for this submission. I've pushed a change that avoids it.

tobydriscoll commented 4 years ago

@drvinceknight I think I have addressed the reviewers' comments. Both were quite helpful.

drvinceknight commented 4 years ago

@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:

drvinceknight commented 4 years ago

@dpsanders and @dlfivefifty could you confirm you're happy with the requested modifications? (Thanks again for your time :+1:)

dpsanders commented 4 years ago

Good for me, thanks.

dpsanders commented 4 years ago

The References heading in the PDF didn't come out right.

tobydriscoll commented 4 years ago

@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:

tobydriscoll commented 4 years ago

@dpsanders I needed to put in a blank line. References header is ok now.

drvinceknight commented 4 years ago

@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:

drvinceknight commented 4 years ago

@dlfivefifty could you confirm you're now happy with the paper?

Good for me, thanks.

@dpsanders I've taken the liberty to tick the two remaining boxes on your check list ("Community guidelines", "State of the field" and "Reference").

dlfivefifty commented 4 years ago

Two comments:

  1. The citation for IntervalSets.jl gives Huybrechs and myself (Olver) too much credit: we’ve made contributions but it was created by @mronian, with many contributions by @timholy
  2. The use of “.jl” in package names should be consistent between “ComplexRegions” and “IntervalSets.jl and DomainSets.jl”.
tobydriscoll commented 4 years ago

Updates:

  1. I have avoided trying to summarize credit for IntervalSets. The paper links to it, where the full history is easy to see.

  2. I have decided to go without .jl on all the package names. It's an editorial choice, but seeing it over and over is really clunky.

tobydriscoll commented 4 years ago

@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:

dlfivefifty commented 4 years ago

Ok one last comment: add references for DomainSets and IntervalSets as otherwise it’s not clear how to find them. This could just be links to the github pages

tobydriscoll commented 4 years ago

My mistake. I thought adding them as markdown links would get through. I put them in the Refs section instead.

@whedon generate pdf

tobydriscoll commented 4 years ago

@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:

tobydriscoll commented 4 years ago

sigh Forgot to brace my capital Js. Trying again.

tobydriscoll commented 4 years ago

@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:

drvinceknight commented 4 years ago

@whedon check references

whedon commented 4 years ago
Attempting to check references...
whedon commented 4 years ago

OK DOIs

- 10/gf9fzf is OK
- 10.1109/tmi.2004.831226 is OK
- 10.1137/060659119 is OK
- 10.1093/imamat/hxw028 is OK

MISSING DOIs

- https://doi.org/10.2514/3.9819 may be missing for title: Wind-tunnel wall corrections on a two-dimensional plate by conformal mapping
- https://doi.org/10.1006/jcph.1993.1175 may be missing for title: Numerical Conformal Mapping Methods for Exterior Regions with Corners
- https://doi.org/10.1016/0022-247x(79)90086-6 may be missing for title: Capacitance and the conformal module of quadrilaterals
- https://doi.org/10.21236/ada198706 may be missing for title: Potential flow in channels
- https://doi.org/10.1007/bf01418327 may be missing for title: An integral equation method for the numerical conformal mapping of interior, exterior, and doubly-connected domains
- https://doi.org/10.1016/0377-0427(86)90133-0 may be missing for title: Numerical conformal mapping via the Szegő kernel
- https://doi.org/10.1109/22.156612 may be missing for title: Conformal mapping analyses of microstrips with circular and elliptic cross-sections
- https://doi.org/10.1137/0910031 may be missing for title: A Fast Algorithm for the Numerical Evaluation of Conformal Mappings
- https://doi.org/10.1016/0377-0427(86)90141-x may be missing for title: Conformal mapping solution of Laplace’s equation on a polygon with oblique derivative boundary conditions
- https://doi.org/10.1063/1.331373 may be missing for title: The geometric correction factor for a rectangular Hall plate

INVALID DOIs

- None
drvinceknight commented 4 years ago

@tobydriscoll it looks like a number of DOIs are missing, would you be able to add those please?

@dlfivefifty I believe the references you asked for are included now (but if you could confirm you're happy for this to be accepted that would be great :+1:)