[PRE REVIEW]: NLSE: A Python package to solve the nonlinear Schrödinger equation

editorialbot commented 8 months ago

Submitting author: !--author-handle-->@taladjidi@RMeli<!--end-editor-- Reviewers: @Abinashbunty, @obliviateandsurrender Managing EiC: Kyle Niemeyer

Status

Status badge code:

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

Author instructions

Thanks for submitting your paper to JOSS @taladjidi. Currently, there isn't a JOSS editor assigned to your paper.

@taladjidi if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). You can search the list of people that have already agreed to review and may be suitable for this submission.

Editor instructions

The JOSS submission bot @editorialbot is here to help you find and assign reviewers and start the main review. To find out what @editorialbot can do for you type:

@editorialbot commands

editorialbot commented 8 months ago

Hello human, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf

editorialbot commented 8 months ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/1367-2630/acce5a is OK
- 10.1103/PhysRevA.108.063512 is OK

MISSING DOIs

- No DOI given, and none found for title: Full Optical Control of Quantum Fluids of Light in...

INVALID DOIs

- None

editorialbot commented 8 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.03 s (546.3 files/s, 127936.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          13            209            620           2792
Markdown                         2             89              0            213
TeX                              1              3              0             32
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                            17            302            624           3055
-------------------------------------------------------------------------------

Commit count by author:

    78  Tangui Aladjidi
    27  taladjidi
     4  clpiekarski
     3  ruggiamp@gmail.com
     1  TANGUI ALADJIDI

editorialbot commented 8 months ago

Paper file info:

📄 Wordcount for paper.md is 495

✅ The paper includes a Statement of need section

editorialbot commented 8 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 8 months ago

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

kyleniemeyer commented 8 months ago

@editorialbot invite @RMeli as editor

editorialbot commented 8 months ago

Invitation to edit this submission sent!

RMeli commented 8 months ago

@editorialbot assign me as editor

editorialbot commented 8 months ago

Assigned! @RMeli is now the editor

RMeli commented 8 months ago

Hi @alexbuccheri 👋

I know you recently reviewed for JOSS (thanks for that, really appreciated!). But would you be interested in reviewing this submission as well? (Totally fine if you have to decline.)

Thank you in advance!

RMeli commented 8 months ago

Hi @rashatwi 👋

Would you be interested in reviewing this submission for JOSS (Journal of Open Source Software)?

You can find more information about the submission at the top of this Github issue #6509.

The Journal of Open Source Software (JOSS) is a developer friendly journal for research software packages, with a formal peer-review process that is designed to improve the quality of the software submitted.

Thank you in advance!

RMeli commented 8 months ago

Hi @PhilipVinc 👋

Would you be interested in reviewing this submission for JOSS (Journal of Open Source Software)?

You can find more information about the submission at the top of this Github issue #6509.

The Journal of Open Source Software (JOSS) is a developer friendly journal for research software packages, with a formal peer-review process that is designed to improve the quality of the software submitted.

Thank you in advance!

PhilipVinc commented 8 months ago

Sorry, reviewing too many things right now...

Abinashbunty commented 8 months ago

@RMeli This paper is interesting for me to review. Let me know if I can volunteer for the same. 😊

RMeli commented 8 months ago

@PhilipVinc no worries, thank you for letting me know!

RMeli commented 8 months ago

@editorialbot add @Abinashbunty as reviewer

Yes, thanks for volunteering!

editorialbot commented 8 months ago

@Abinashbunty added to the reviewers list!

AlexBuccheri commented 8 months ago

Hi @RMeli , apologies but I don't have time for this one (and also don't know anything about the nonlinear Schrödinger equation 😅)

RMeli commented 8 months ago

No worries, thanks @AlexBuccheri!

RMeli commented 8 months ago

@taladjidi thank you for submitting to JOSS. While I look for reviewers, I started having a look at the code base and I noticed the following:

In the test folder, I could not find any test framework; looking more closely, the tests don't seem to test anything (there are no assertions) and I was not able to locate any unit tests. Am I missing something?
The nlse.py file is a single massive Python file of 2500+ lines of code. Calculations and analysis seems to be very tightly coupled within classes, and it seems that there is a lot of code duplication. There is also a lot of branching in the code (a lot of nested if/else), that makes the logic difficult to follow.

Could you please address the previous two points (proper testing, decoupling between calculations and analysis, removal of pervasive code duplication)?

taladjidi commented 8 months ago

Hi, thank you for getting back to me so quickly. Trying to slowly work my way from "physicist" code to actual code 😅 I'll get right on it. I'll add a new comment once this is done.

RMeli commented 8 months ago

Trying to slowly work my way from "physicist" code to actual code

@taladjidi no worries, I totally get where you are coming from. If you need any suggestions do let me know.

taladjidi commented 8 months ago

@RMeli I significantly refactored the code:

I split all the classes into separate files, using inheritance as much as possible to limit code duplication
Added functions in order to make the logic more legible
Wrote a series of tests using Pytest, with the corresponding GitHub action At the moment though, one test fails inexplicably in the GitHub action, while it passes on all the computers I have access to in the lab (Windows, MacOs or Linux). I did not manage to diagnosticate the issue for now ... I hope this adresses most of your remarks 😀

RMeli commented 8 months ago

@editorialbot check repository

editorialbot commented 8 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.03 s (892.2 files/s, 115054.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          22            222            469           2450
Markdown                         2             80              0            182
YAML                             2              3              4             37
TeX                              1              3              0             32
-------------------------------------------------------------------------------
SUM:                            27            308            473           2701
-------------------------------------------------------------------------------

Commit count by author:

   131  Tangui Aladjidi
    27  taladjidi
     4  clpiekarski
     3  ruggiamp@gmail.com
     1  TANGUI ALADJIDI

editorialbot commented 8 months ago

Paper file info:

📄 Wordcount for paper.md is 495

✅ The paper includes a Statement of need section

editorialbot commented 8 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

RMeli commented 8 months ago

Thanks @taladjidi. I'll start looking for additional reviewers.

At the moment though, one test fails inexplicably in the GitHub action

Loos like norm ends up being 0.0. Do you have any update on troubleshooting the issue?

taladjidi commented 8 months ago

Hi, yes I saw. I did not manage to replicate the issue. I thought it might be a cache issue since I do a lot of jitting, but even when I clear all caches, I do not manage to replicate this zero norm problem (I created an environment cloning the packages versions used by GitHub). I'll spend some more energy on this today !

taladjidi commented 8 months ago

@RMeli all good now ! It was an overflow error due to some edge case in the solver's resolution. Now everything passes 🥳

RMeli commented 8 months ago

Thanks for the update @taladjidi. Some of the tests you added don't seem very meaningful (test_build_propagator just uses the same expression as the implementation), but I'll leave it to the reviewers to comment further. Thanks for adding proper testing.

RMeli commented 8 months ago

Hi @HugoStrand 👋

Would you be interested in reviewing this submission for JOSS (Journal of Open Source Software)?

You can find more information about the submission at the top of this Github issue #6509.

The Journal of Open Source Software (JOSS) is a developer friendly journal for research software packages, with a formal peer-review process that is designed to improve the quality of the software submitted.

Thank you in advance!

RMeli commented 8 months ago

Hi @obliviateandsurrender 👋

Would you be interested in reviewing this submission for JOSS (Journal of Open Source Software)?

You can find more information about the submission at the top of this Github issue #6509.

The Journal of Open Source Software (JOSS) is a developer friendly journal for research software packages, with a formal peer-review process that is designed to improve the quality of the software submitted.

Thank you in advance!

obliviateandsurrender commented 8 months ago

Hey @RMeli, sure thing!

RMeli commented 8 months ago

@editorialbot assign @obliviateandsurrender as reviewer

Thank you!

editorialbot commented 8 months ago

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

@editorialbot commands

RMeli commented 8 months ago

@editorialbot add @obliviateandsurrender as reviewer

editorialbot commented 8 months ago

@obliviateandsurrender added to the reviewers list!

RMeli commented 8 months ago

Thank you @Abinashbunty and @obliviateandsurrender for agreeing to review for JOSS. I'll soon start the review process, closing this PRE-REVIEW issue and opening a REVIEW issue for the actual review.

If it's your first time reviewing for JOSS, please have a look at the following pages:

A good way to review is to open issues in the software repository, and link them to the (soon-to-be-open) review issue.

Do not hesitate to ping me with any questions you might have.

RMeli commented 8 months ago

@editorialbot start review

editorialbot commented 8 months ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/6607.

openjournals / joss-reviews

[PRE REVIEW]: NLSE: A Python package to solve the nonlinear Schrödinger equation #6509

Status