search

openjournals / joss-reviews

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

[REVIEW]: eigentools: A Python package for studying differential eigenvalue problems with an emphasis on robustness #3079

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @jsoishi (Jeffrey S. Oishi) Repository: https://github.com/DedalusProject/eigentools Version: 2.2106 Editor: @dpsanders Reviewer: @ketch, @caropen Archive: 10.5281/zenodo.4968601

: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/c7fac08ba47b750d49799a2aac4e80a6"><img src="https://joss.theoj.org/papers/c7fac08ba47b750d49799a2aac4e80a6/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/c7fac08ba47b750d49799a2aac4e80a6/status.svg)](https://joss.theoj.org/papers/c7fac08ba47b750d49799a2aac4e80a6)

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

@ketch & @caropen, 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 @dpsanders 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 @ketch

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @caropen

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ketch, @caropen 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 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.25 s (76.4 files/s, 12566.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          11            346            568            906
Jupyter Notebook                 2              0            905            184
reStructuredText                 3             41             37             54
Markdown                         1             11              0             31
YAML                             1              6              0             16
make                             1              4              6             10
-------------------------------------------------------------------------------
SUM:                            19            408           1516           1201
-------------------------------------------------------------------------------

Statistical information for the repository 'a844dd697379390b04ca8b67' was
gathered on 2021/03/02.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Benjamin Brown                   1             1              3            0.07
Evan Anders                     39          1003            482           24.32
J. S. Oishi                     57          1868           1555           56.06
J.S. Oishi                      25           695             87           12.81
Keaton J. Burns                  1             3              0            0.05
Susan Clark                      2           397             12            6.70

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
Evan Anders                 147           14.7         41.7               17.01
J. S. Oishi                1629           87.2          6.7               11.36
Susan Clark                  44           11.1         59.7                9.09
whedon commented 3 years ago

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

Can't find any papers to compile :-(

jsoishi commented 3 years ago

@whedon commands

whedon commented 3 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
jsoishi commented 3 years ago

@whedon generate pdf from branch joss_paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...
whedon commented 3 years ago

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

whedon commented 3 years ago

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

whedon commented 3 years ago

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

ketch commented 3 years ago

I was able to install the software but it doesn't seem to work. I have raised an issue.

caropen commented 3 years ago

I was able to run the software and all provided examples. I could check off all items except the one asking for "Community guidelines", which I could not find. Issue: https://github.com/DedalusProject/eigentools/issues/7

dpsanders commented 3 years ago

Yes, feel free to open an issue in the repo. It is useful to link the issues here too. Thanks!

jsoishi commented 3 years ago

@caropen Thanks! We've addressed the community guidelines in a commit this afternoon the eigentools repository. Please let us know if there's anything else you'd like to see us do to address this.

ketch commented 3 years ago

This looks like a really great package that I myself may use at some point! Below are the issues I found.

I get some errors when running one of the documentation examples and I raised an issue for that.

Some minor things that should be corrected in the paper:

Authorship: the package seems to be written almost exclusively by lead author. What are the contributions of others? (I'm just basing this on Github's commit data)

Installation: the installation instructions at https://eigentools.readthedocs.io/en/latest/pages/installation.html read something like "just do this easy step 1. All done. But note that your installation won't actually work because you didn't do step zero. Now we will tell you about step zero." I think they should be rewritten more in the form:

Step zero: install Dedalus Step one: pip install eigentools Done.

bpbrown commented 3 years ago

@ketch and JOSS editors, I wanted to help answer the authorship question.

We often work in team-coding environments, with several of us simultaneously working on the code while one person makes the logged commits. Keaton Burns and myself (Ben Brown) jointly worked with Jeff on the refactor and on the JOSS example problems and paper figures, with Jeff Oishi writing and committing the code. Daniel Lecoanet and Geoffrey Vasil helped with the pseudospectra functionality. Evan Anders did early work on parameter space searching, laying the foundation for the refactored version.

There are many early commits by both Evan Anders and Susan Clark in the commit history, but these commits are not yet linked to their github identities. This work had started in a mercurial repo on bitbucket, before transitioning to git on github, and while that history is there, that transition happened after Evan and Susan's primary involvement in the project. Using "git shortlog --summary --numbered --email", Evan is the second highest contributor by commits after Jeff Oishi, and Susan's commits are also present. Their github accounts are not yet linked to these older academic e-mails, and we are working with them to resolve that.

ketch commented 3 years ago

@bpbrown Thanks for the clarification. That all makes sense. I've checked off the corresponding box.

dpsanders commented 3 years ago

👋 @ketch and @caropen: I see that you have both checked off all the boxes. Are you done with your reviews and happy to proceed with publication? Thanks!

ketch commented 3 years ago

@dpsanders I made some minor suggestions in a comment just a few lines above this one. Once that is resolved, I can sign off on this.

caropen commented 3 years ago

Yes, I am happy to proceed with publication.

jsoishi commented 3 years ago

@dpsanders @ketch @caropen Great! I'll fix @ketch's suggestions this afternoon.

Jeff, on behalf of the authors

On Mon, Apr 26, 2021 at 2:48 AM caropen @.***> wrote:

Yes, I am happy to proceed with publication.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3079#issuecomment-826554964, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7SHYJ4O5OXMRLXU3KFD6DTKUEEJANCNFSM4YPGFCIQ .

jsoishi commented 3 years ago

@dpsanders @ketch @caropen I've responded to all of @ketch's suggestions. The additions to the paper can be found in the joss_paper branch on github; the corrections to the documentation and contributing.md are on master.

Thanks for the really helpful suggestions.

dpsanders commented 3 years ago

@whedon generate pdf from branch joss_paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...
whedon commented 3 years ago

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

dpsanders commented 3 years ago

@jsoishi: Following on from @ketch's comment above, I still find the installation instructions in the repo unclear.

I suggest providing a "quick install" guide in the eigentools repository that provides the simplest instructions for installing all required dependencies, and refer to the Dedalus repository for more advanced instructions if necessary.

I am not up to date with the Python packaging situation, but it seems to me (naively?) like it should be possible to make

conda add eigentools

install all required dependencies?

jsoishi commented 3 years ago

@dpsanders Unfortunately, conda has limited support for dependencies requiring locally-built and optimized libraries (e.g. MPI, FFTW provided by supercomputing centers), as Dedalus does. For now, this prevents Dedalus from distributing binaries via conda and thus the same for eigentools. eigentools remains easily pip installable as long as Dedalus is properly installed.

dpsanders commented 3 years ago

@jsoishi: Apologies for the delay.

Here is my attempt to install dedalus following then conda installation instructions linked from the repo. I first installed miniconda using homebrew, and then installed dedalus with the unmodified conda script.

I get the following errors when I run the tests:

>                   val = math.factorial(un[0])
E                   TypeError: 'numpy.float128' object cannot be interpreted as an integer

/usr/local/Caskroom/miniconda/base/envs/dedalus/lib/python3.8/site-packages/scipy/special/_basic.py:2352: TypeError
------------------------------ Captured log call -------------------------------
DEBUG    domain:domain.py:55 Global coeff shape: [128]
DEBUG    distributor:distributor.py:92 Mesh: []
DEBUG    problems:problems.py:127 Parsing Eqn 0
DEBUG    problems:problems.py:146   Condition: True
DEBUG    problems:problems.py:147   LHS string form: dx(u) + 2*x*u
DEBUG    problems:problems.py:148   RHS string form: 0
DEBUG    problems:problems.py:155   LHS object form: dx(u) + ((2*x)*u)
DEBUG    problems:problems.py:156   RHS object form: 0
DEBUG    problems:problems.py:308   L linear form: dx(u) + ((2*x)*u)
DEBUG    problems:problems.py:127 Parsing Eqn 1
DEBUG    problems:problems.py:146   Condition: True
DEBUG    problems:problems.py:147   LHS string form: integ(u)
DEBUG    problems:problems.py:148   RHS string form: sqrt(pi)
DEBUG    problems:problems.py:155   LHS object form: integ_x(u)
DEBUG    problems:problems.py:156   RHS object form: sqrt(pi)
DEBUG    problems:problems.py:308   L linear form: integ_x(u)
DEBUG    solvers:solvers.py:180 Beginning LBVP instantiation
DEBUG    basis:basis.py:1064 Building Hermite MMT matrices for (gsize, csize, env) = (128, 128, False)

Full output of installation and tests here:

https://gist.github.com/e1a8bb1e691014ee95a0e0a5e96f2c5d

kburns commented 3 years ago

Hi @dpsanders, this issue has been fixed on the dedalus development branch and is slated to be included in the next release (https://github.com/DedalusProject/dedalus/issues/133). It shouldn't impact any of the eigentools tests or applications for now, other than those hitting the same upstream edge cases.

dpsanders commented 3 years ago

OK thanks.

Where is the narrative documentation on how to use the package? I thought I saw some but now I can't find it. This should be linked from the GitHub repo.

dpsanders commented 3 years ago

It's here but doesn't seem to be linked from anywhere.

dpsanders commented 3 years ago

In particular, not from https://eigentools.readthedocs.io/en/latest/ !

kburns commented 3 years ago

I think there was just some rendering bug on RTD -- I triggered a rebuild, and it looks like all the docs are appearing there now.

jsoishi commented 3 years ago

hi @dpsanders, is there anything else you'd like us to do?

danielskatz commented 3 years ago

👋 @dpsanders - It looks like everything is checked off from the reviewers - can you help me understand what this submission is waiting for at this point?

dpsanders commented 3 years ago

Apologies for the delay. I will be doing a final look tomorrow.

dpsanders commented 3 years ago

Everything looks great to me!

dpsanders commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago

No archive DOI set. Exiting...

dpsanders commented 3 years ago

👋 @jsoishi: Could you please archive the package, e.g. at Zenodo, and report back here the corresponding version of the software and archive link. Please make sure that the title and author data for the archive correspond exactly to those of the paper. Thanks!

jsoishi commented 3 years ago

@dpsanders Here you go:

https://zenodo.org/record/4968601#.YMpIOJpKirQ

thanks! Let me know if there's anything else that needs to be done.

dpsanders commented 3 years ago

Thanks. Is the version 2.2106?

jsoishi commented 3 years ago

Yes!

On Wed, Jun 16, 2021, 19:46 David P. Sanders @.***> wrote:

Thanks. Is the version 2.2106?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3079#issuecomment-862805284, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7SHYNMZGXVEJ4OUHRHUG3TTEZVVANCNFSM4YPGFCIQ .

dpsanders commented 3 years ago

@whedon set archive as 2.2106

whedon commented 3 years ago

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

@whedon commands
dpsanders commented 3 years ago

@whedon commands

whedon commented 3 years ago

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

# Re-invite a reviewer (if they can't update checklists)
@whedon re-invite @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

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# All commands can be run on a non-default branch, to do this pass a custom 
# branch name by following the command with `from branch custom-branch-name`.
# For example:

# Compile the paper
@whedon generate pdf

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

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

# Ask Whedon to do a dry run of accepting the paper and depositing with Crossref
@whedon recommend-accept

# 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

EiC TASKS

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

# Reject a paper
@whedon reject

# Withdraw a paper
@whedon withdraw

# Ask Whedon to actually accept the paper and deposit with Crossref
@whedon accept deposit=true
dpsanders commented 3 years ago

@whedon set 2.2106 as version

whedon commented 3 years ago

OK. 2.2106 is the version.

dpsanders commented 3 years ago

@whedon set https://zenodo.org/record/4968601#.YMpIOJpKirQ as archive