search

openjournals / joss-reviews

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

[REVIEW]: PyAFBF: a Python library to sample image textures from the anisotropic fractional Brownian field. #3821

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@fjprichard<!--end-author-handle-- (Frédéric Richard) Repository: https://github.com/fjprichard/PyAFBF/ Branch with paper.md (empty if default branch): main Version: v0.1.5 Editor: !--editor-->@pibion<!--end-editor-- Reviewers: @vitorsr, @janmandel Archive: 10.5281/zenodo.6860333

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@vitorsr & @janmandel, 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 @pibion know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @vitorsr

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @janmandel

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @vitorsr, @janmandel it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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 2 years ago

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

 Can't find any papers to compile :-(
whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.39 s (314.6 files/s, 100487.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                      14           2404           2467           9203
HTML                            29           3865             87           9038
SVG                              2              0              0           2672
Python                          21            731           1868           1455
reStructuredText                38           1241           1226           1337
CSS                              8            199             46           1145
YAML                             1              5              2             24
Jupyter Notebook                11              0            572             22
-------------------------------------------------------------------------------
SUM:                           124           8445           6268          24896
-------------------------------------------------------------------------------

Statistical information for the repository '1f864ebc8ca77589abe733e5' was
gathered on 2021/10/12.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Frederic Richard                12         58153          38934          100.00

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
Frederic Richard          19219           33.0          0.0               20.93
pibion commented 2 years ago

@vitorsr @janmandel could you both verify that you can check and uncheck items on the checklist at the top of this thread and also comment?

pibion commented 2 years ago

@fjprichard, @arfon do you have any thoughts about why the pdf compilation is failing? Is the pdf in a non-standard location?

vitorsr commented 2 years ago

@vitorsr @janmandel could you both verify that you can check and uncheck items on the checklist at the top of this thread and also comment?

I have partially filled out my review based on a previous peruse.

Is the pdf in a non-standard location?

Yes, paper.md seems to be on a separate branch called joss, in a folder of the same name:

https://github.com/fjprichard/PyAFBF/blob/joss/joss/paper.md

vitorsr commented 2 years ago

Major comments:

I am unable to replicate the auto_examples (Example gallery) as I have not found enough information to replicate the development environment and locally generate the aforementioned documentation.

The same holds for the nose tests.

Minor comments:

The API is incredibly well documented. Nice-to-haves for researchers and alike such as the inclusion of a glossary are of immense help. However, the repository documentation itself (mainly referring to README.rst) is not prominent enough to provide a good overview of the package.

Everything is otherwise stellar.

whedon commented 2 years ago

:wave: @vitorsr, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @janmandel, please update us on how your review is going (this is an automated reminder).

fjprichard commented 2 years ago

Dear visitorsr,

Thank you for your review. Please find below some answers to your comments.

Major comments:

"I am unable to replicate the auto_examples (Example gallery) as I have not found enough information to replicate the development environment and locally generate the aforementioned documentation.

The same holds for the nose tests."

Information about the environment are in the setup file (packages required and versions). Would you suggest to have it somewhere else?

Minor comments:

"The API is incredibly well documented. Nice-to-haves for researchers and alike such as the inclusion of a glossary are of immense help. However, the repository documentation itself (mainly referring to README.rst) is not prominent enough to provide a good overview of the package."

I added the content of the documentation in the README file.

"Everything is otherwise stellar." Thank you.

vitorsr commented 2 years ago

Hi,

Thanks for addressing my comments.

Information about the environment are in the setup file (packages required and versions). Would you suggest to have it somewhere else?

Yes. I am not sure if the following is clear, however please do note that the development requirements are a superset of the package requirements. The former includes packages for tests, coverage, documentation etc., to name a few development requirements. Moreover please note that I can only infer that nose is a development requirement because setup.cfg has an entry called nosetests.

A common heuristic to determine precisely which are the development requirements is to match your environment from scratch, noting which packages you require to run tests, coverage, documentation etc. Commonly this information is included either in a pip-compatible requirements-dev.txt file, in a Pipfile, in a documentation entry addressing contributors, or with the setuptools extras_requires optional dependencies keyword.

Properly defining development requirements is useful in the open source community to open doors for contributors. As I mentioned, I cannot reproduce your environment as there are no steps on how to do so. Critically, this means I am not able to run your tests. I can verify that your code is sound, but I cannot run your test suite.

fjprichard commented 2 years ago

Dear Vitor,

Thank you for your explanation. I did some corrections on the package to improve these points:

I hope this will help contributors to reproduce the environment.

Best regards, Frederic

janmandel commented 2 years ago

👋 @janmandel, please update us on how your review is going (this is an automated reminder).

I am at the stage of checking and unchecking boxes. I did run the software back in October but I missed this issue then, any relevant messages got buried in an avalanche of notifications from this repository. Obvously I found it now and I am back at it! Sorry about that.

fjprichard commented 2 years ago

Dear @vitorsr , I wonder if you received my answers to your comments sent on december, 8th. Best regards, Frederic

vitorsr commented 2 years ago

Dear professor Richard,

Thanks for notifying me for I had not received your answers. I apologize if this caused any delay. I have now updated the review checklist.

This concludes my review.

My kind regards,

Vítor

On Fri, Feb 4, 2022 at 8:59 AM Frederic Richard @.***> wrote:

Dear @vitorsr https://github.com/vitorsr , I wonder if you received my answers to your comments sent on december, 8th. Best regards, Frederic

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3821#issuecomment-1029923476, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW2VYMIM6BZPGHOWDP4LHTUZO5TDANCNFSM5F3WZOFQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

janmandel commented 2 years ago

!pip install PyAFBF per the documentation errors out on Google Colab complaining about imgaug version. To make it work,

!pip install imgaug==0.2.6 !pip install PyAFBF

janmandel commented 2 years ago

I copied most examples from the documentation to a google colab notebook and they work fine. Downloading separate notebook files would be less convenient (too many files), besides I would have to add the installation of the prerequisites (see previous comment) in each notebook.

It was unclear to me how use this package to get the values of a generated random field. That's how I would expect to use it. There is ndarray y in a part of the API, but all examples in the documentation I looked at end with Display(). Would it be possible to provide a simple example how to get a generated array, and then display in by standard graphics calls, please? I apologize if that was provided already and I missed it. Also, it would be nice to have the TOC point to such information.

pibion commented 2 years ago

@vitorsr thank you!

fjprichard commented 2 years ago

@janmandel Thank you for your valuable comments. I modified the docs

pibion commented 2 years ago

@janmandel just checking in on the open review items in your list. Has @fjprichard addressed all your issues, or is there more work that needs to be done?

pibion commented 2 years ago

@janmandel @fjprichard are there outstanding review issues? I see several items that aren't checked off; are any of those addressed by @fjprichard updates?

fjprichard commented 2 years ago

@janmandel I think I answered to all your comments. Is there any left ?

janmandel commented 2 years ago

It's fine thank you. I had to step away from testing for few weeks but then it was not straightforward to find my way back here resulting in a delay.

pibion commented 2 years ago

@janmandel thank you for the update!

pibion commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

Checking the BibTeX entries failed with the following error: 

No paper file path
pibion commented 2 years ago

@arfon this paper is in a non-standard location , do you have a suggestion for how to proceed?

@fjprichard, @vitorsr pointed out:

Yes, paper.md seems to be on a separate branch called joss, in a folder of the same name: https://github.com/fjprichard/PyAFBF/blob/joss/joss/paper.md

We need the paper to build, so you may need to move paper.md so it conforms to the expected location.

pibion commented 2 years ago

@arfon on the form (https://joss.theoj.org/papers/new) it does look like you can set the branch that contains paper.md - can this be done after the paper is submitted?

danielskatz commented 2 years ago

@editorialbot commands

this should help you

editorialbot commented 2 years ago

Hello @danielskatz, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

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

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for version
@editorialbot set v1.0.0 as version

# Set a value for archive
@editorialbot set 10.21105/zenodo.12345 as archive

# Set a value for branch
@editorialbot set joss-paper as branch

# Reject paper
@editorialbot reject

# Withdraw paper
@editorialbot withdraw

# Invite an editor to edit a submission (sending them an email)
@editorialbot invite @(.*) as editor

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Accept and publish the paper
@editorialbot accept

# Update data on an accepted/published paper
@editorialbot reaccept

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Open the review issue
@editorialbot start review
pibion commented 2 years ago

@editorialbot set joss-paper as joss

editorialbot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

pibion commented 2 years ago

@editorialbot set joss as branch

editorialbot commented 2 years ago

Done! branch is now joss

pibion commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cageo.2004.03.012 is OK
- 10.18637/jss.v023.i01 is OK
- 10.1051/ps:2007031 is OK
- 10.1007/s11222-017-9785-z is OK
- 10.1016/j.spasta.2016.02.001 is OK
- 10.1016/j.proenv.2015.07.100 is OK
- 10.5705/ss.202014.0077 is OK
- 10.1007/s10851-009-0181-y is OK
- 10.1007/978-3-642-19604-1_3 is OK
- 10.1051/proc/2009008 is OK
- 10.1080/10618600.2014.946603 is OK
- 10.1016/j.spa.2020.01.012 is OK
- 10.1016/s0167-7152(99)00171-6 is OK
- 10.1109/icip.2014.7026229 is OK
- 10.1007/s00041-003-0012-2 is OK

MISSING DOIs

- 10.4171/rmi/217 may be a valid DOI for title: Elliptic Gaussian random processes

INVALID DOIs

- None
pibion commented 2 years ago

@fjprichard could you take a look at the missing DOI and see if the suggested DOI is correct?

pibion commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pibion commented 2 years ago

@fjprichard minor suggestions for the paper:

fjprichard commented 2 years ago

I corrected the doi

fjprichard commented 2 years ago

I made the corrections.

fjprichard commented 2 years ago

point_rightpage_facing_up Download article proof page_facing_up View article proof on GitHub page_facing_up point_left

I read the proof. It's fine.

fjprichard commented 2 years ago

I think the review process is finished. What's next ?

pibion commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cageo.2004.03.012 is OK
- 10.18637/jss.v023.i01 is OK
- 10.1051/ps:2007031 is OK
- 10.1007/s11222-017-9785-z is OK
- 10.1016/j.spasta.2016.02.001 is OK
- 10.1016/j.proenv.2015.07.100 is OK
- 10.5705/ss.202014.0077 is OK
- 10.1007/s10851-009-0181-y is OK
- 10.1007/978-3-642-19604-1_3 is OK
- 10.1051/proc/2009008 is OK
- 10.1080/10618600.2014.946603 is OK
- 10.1016/j.spa.2020.01.012 is OK
- 10.1016/s0167-7152(99)00171-6 is OK
- 10.1109/icip.2014.7026229 is OK
- 10.4171/rmi/217 is OK
- 10.1007/s00041-003-0012-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
pibion commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pibion commented 2 years ago

@fjprichard I have a few more notes on minor corrections to the paper - I'm working on those now and will have them up tonight. Then I'll ask you to do a code release and generate a DOI (zenodo is a popular choice for that) and we'll recommend acceptance.

Apologies for not getting you the complete set of edits earlier, I'm moving this week and things have been hectic!

pibion commented 2 years ago