Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
editorialbot opened this issue Mar 20, 2024 · 42 comments
Assignees
Labels

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Mar 20, 2024

Submitting author: @taladjidi (Tangui Aladjidi)
Repository: https://github.com/Quantum-Optics-LKB/NLSE
Branch with paper.md (empty if default branch):
Version: 2.0.0
Editor: @RMeli
Reviewers: @Abinashbunty, @obliviateandsurrender
Managing EiC: Kyle Niemeyer

Status

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 editorialbot added pre-review Track: 3 (PE) Physics and Engineering labels Mar 20, 2024
@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Paper file info:

📄 Wordcount for paper.md is 495

✅ The paper includes a Statement of need section

@editorialbot
Copy link
Collaborator Author

License info:

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

@editorialbot
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@kyleniemeyer
Copy link

@editorialbot invite @RMeli as editor

@editorialbot
Copy link
Collaborator Author

Invitation to edit this submission sent!

@RMeli
Copy link

RMeli commented Mar 20, 2024

@editorialbot assign me as editor

@editorialbot
Copy link
Collaborator Author

Assigned! @RMeli is now the editor

@RMeli
Copy link

RMeli commented Mar 20, 2024

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
Copy link

RMeli commented Mar 20, 2024

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
Copy link

RMeli commented Mar 20, 2024

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
Copy link

Sorry, reviewing too many things right now...

@Abinashbunty
Copy link

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

@RMeli
Copy link

RMeli commented Mar 21, 2024

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

@RMeli
Copy link

RMeli commented Mar 21, 2024

@editorialbot add @Abinashbunty as reviewer

Yes, thanks for volunteering!

@editorialbot
Copy link
Collaborator Author

@Abinashbunty added to the reviewers list!

@AlexBuccheri
Copy link

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
Copy link

RMeli commented Mar 21, 2024

No worries, thanks @AlexBuccheri!

@RMeli
Copy link

RMeli commented Mar 21, 2024

@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
Copy link

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
Copy link

RMeli commented Mar 22, 2024

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
Copy link

@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
Copy link

RMeli commented Apr 4, 2024

@editorialbot check repository

@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Paper file info:

📄 Wordcount for paper.md is 495

✅ The paper includes a Statement of need section

@editorialbot
Copy link
Collaborator Author

License info:

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

@RMeli
Copy link

RMeli commented Apr 4, 2024

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
Copy link

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
Copy link

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

@RMeli
Copy link

RMeli commented Apr 10, 2024

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
Copy link

RMeli commented Apr 10, 2024

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
Copy link

RMeli commented Apr 10, 2024

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
Copy link

Hey @RMeli, sure thing!

@RMeli
Copy link

RMeli commented Apr 10, 2024

@editorialbot assign @obliviateandsurrender as reviewer

Thank you!

@editorialbot
Copy link
Collaborator Author

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

@editorialbot commands

@RMeli
Copy link

RMeli commented Apr 10, 2024

@editorialbot add @obliviateandsurrender as reviewer

@editorialbot
Copy link
Collaborator Author

@obliviateandsurrender added to the reviewers list!

@RMeli
Copy link

RMeli commented Apr 12, 2024

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
Copy link

RMeli commented Apr 12, 2024

@editorialbot start review

@editorialbot
Copy link
Collaborator Author

OK, I've started the review over in #6607.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants