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]: SphericalScattering: A Julia Package for Electromagnetic Scattering from Spherical Objects #5820

Closed editorialbot closed 11 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@HoBeZwe<!--end-author-handle-- (Bernd Hofmann) Repository: https://github.com/HoBeZwe/SphericalScattering.jl Branch with paper.md (empty if default branch): Version: v0.6.0 Editor: !--editor-->@pdebuyl<!--end-editor-- Reviewers: @iliarasskazov, @berquist Archive: 10.5281/zenodo.10211642

Status

status

Status badge code:

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

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

@iliarasskazov & @berquist, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @pdebuyl 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 ✨

Checklists

📝 Checklist for @berquist

📝 Checklist for @iliarasskazov

editorialbot commented 1 year ago

Hello humans, 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 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.12 s (546.4 files/s, 193401.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
SVG                             13              5              5          17270
Julia                           30           1445            320           3114
Markdown                        14            343              0            873
TeX                              1             18              0            163
YAML                             5              1              4            108
TOML                             3              9              0             33
CSS                              1              1              0              4
-------------------------------------------------------------------------------
SUM:                            67           1822            329          21565
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 555

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

OK DOIs

- 10.1017/CBO9780511574498 is OK
- 10.1109/TAP.2023.3247896 is OK
- 10.1109/TAP.2023.3234704 is OK
- 10.23919/EuCAP57121.2023.10133154 is OK
- 10.1109/OJAP.2021.3121097 is OK
- 10.1137/141000671 is OK
- 10.21105/joss.00691 is OK
- 10.5281/zenodo.7949263 is OK
- 10.5281/zenodo.248729 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.5281/zenodo.1213225 is OK

MISSING DOIs

- None

INVALID DOIs

- None
pdebuyl commented 1 year ago

@iliarasskazov @bergquist please check the information text at the top of this page.

As the review takes place into this discussion page, feel free to provide a partial review at some point or to ask clarifications to the author here.

Some reviewers also open issues at the repository under review. This is fine for bug reports or requests about the documentation, for instance. In that case, please report the resulting bugfix or other result from the discussion here so that I can get proper notification.

editorialbot commented 1 year ago

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

berquist commented 1 year ago

@pdebuyl Careful, my last name doesn't have the g :)

berquist commented 1 year ago

Review checklist for @berquist

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

iliarasskazov commented 1 year ago

Review checklist for @iliarasskazov

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

pdebuyl commented 1 year ago

@berquist (no g this time) @iliarasskazov thanks both for starting the review. Gentle reminder about it :-)

HoBeZwe commented 1 year ago

Thank you already for everybody's effort to review so far.

We have added a section Application Examles to the documentation to further improve the usability and also to make clearer what kind of tests are performed.

We have also updated the paper and discuss now a wider range of related software including implementations in C++, Python, and MATLAB.

Concerning the Zenodo reference: To our understanding, it will be added to the paper only after the review (in case it is positive of course). But please correct me if I am wrong here.

HoBeZwe commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

HoBeZwe commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1017/CBO9780511574498 is OK
- 10.1109/TAP.2023.3247896 is OK
- 10.1109/TAP.2023.3234704 is OK
- 10.23919/EuCAP57121.2023.10133154 is OK
- 10.1109/APS/URSI47566.2021.9703799 is OK
- 10.1109/AP-S/USNC-URSI47032.2022.9886833 is OK
- 10.23919/EuCAP53622.2022.9769650 is OK
- 10.1109/USNC-URSI52151.2023.10238214 is OK
- 10.1109/OJAP.2021.3121097 is OK
- 10.1137/141000671 is OK
- 10.21105/joss.00691 is OK
- 10.5281/zenodo.7949263 is OK
- 10.5281/zenodo.248729 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.5281/zenodo.1213225 is OK
- 10.5281/zenodo.5703291 is OK
- 10.1016/j.jqsrt.2017.05.010 is OK
- 10.1364/OSAC.399979 is OK

MISSING DOIs

- None

INVALID DOIs

- None
pdebuyl commented 1 year ago

Hi @berquist first time I see the review in-line with the checklist, good idea.

@HoBeZwe indeed, no worry about the zenodo entry at this time. When the review comes to an end, I'll ask for the suitable version and zenodo entry. (make sure that it matches the article's title and author list).

berquist commented 1 year ago

@HoBeZwe Could you say something here about some of the commented-out test code such as https://github.com/HoBeZwe/SphericalScattering.jl/blob/bebea6b63d8b9eef1f7c108e51d4cb6d06c57559/test/sphericalModes.jl#L39?

berquist commented 1 year ago

For the summary, it isn't clear how the section starting "While there are some applications where canonical scattering problems (...)" is relevant at this particular point in the text, but it seems like it should be connected to the previous point about implementations of semi-analytical solutions. Is that what this package implements, therefore enabling scattering calculations as part of more involved techniques? I think the first paragraph of the Statement of Need says this, so some movement of text between these two parts might clarify the summary.

HoBeZwe commented 1 year ago

@HoBeZwe Could you say something here about some of the commented-out test code such as https://github.com/HoBeZwe/SphericalScattering.jl/blob/bebea6b63d8b9eef1f7c108e51d4cb6d06c57559/test/sphericalModes.jl#L39?

Since the higher-order modes were not stabilized (as pointed out in the documentation) we were a bit lazy in the tests for this excitation. So a good reason to be skeptical here.

To properly fix this, we decided to directly remove the limitation and then upgrade the tests. The latter are now more thorough (especially, the TE scattering case and the TM scattering case).

Concerning, tests like this we added comments to clearly state the origin of the numerical values and the limited usefulness of the tests.

HoBeZwe commented 1 year ago

For the summary, it isn't clear how the section starting "While there are some applications where canonical scattering problems (...)" is relevant at this particular point in the text, but it seems like it should be connected to the previous point about implementations of semi-analytical solutions. Is that what this package implements, therefore enabling scattering calculations as part of more involved techniques? I think the first paragraph of the Statement of Need says this, so some movement of text between these two parts might clarify the summary.

Thanks for pointing this out. We adapted the summary to remove the ambiguity: lines 14-21 and lines 50-51.

HoBeZwe commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

pdebuyl commented 1 year ago

Hi @iliarasskazov gentle reminder about this review. It is nearing completion from "reviewer number 2" :-)

berquist commented 1 year ago

My checklist is done. The revisions to the paper for clarity and the test cleanup are good. "Values are checked against values determined by this package" shows an understanding of the different kinds of reference tests which is often missing. I am now convinced enough that the package correctly implements functionality that the paper makes claims about.

The table of detailed implementation status that is part of the documentation and not just a GitHub issue is something most packages should have. The rest of the documentation, both prose and examples, is clear enough to a scientist but non-practitioner. For the paper itself, there may be some grammar or wording oddities, but I would not nitpick those unless requested since the intent is already clear to me.

I think this paper absolutely belongs in JOSS.

iliarasskazov commented 1 year ago

I've done with the checklist. I really appreciate the hard work of the authors. Apart from the very implementation of theoretical solution, the proper documentation and organization of the code are even more challenging. The authors done a great job in all of the aspects.

The paper is very much suitable for publication in JOSS.

pdebuyl commented 1 year ago

Thanks @berquist for the review! (also very well documented and transparent :-) )

HoBeZwe commented 1 year ago

Thank you, we very much appreciate the positive and constructive feedback.

What are the next steps from here? Should we already make the new release, or are there other things to be completed first?

pdebuyl commented 1 year ago

Hi @HoBeZwe my issue page did not reload so I hadn't seen @iliarasskazov 's comment that actually stand above mine. Sorry about this.

pdebuyl commented 1 year ago

Thanks @iliarasskazov for the review!

pdebuyl commented 1 year ago

@HoBeZwe I will proceed to the editor's after acceptance tasks in the coming days and let you know when an action is required on your side.

pdebuyl commented 1 year ago

Hello @HoBeZwe have started looking at your article + code. Overall it seems to be going ok, except for the comment below on trying the run the code.

I always try to run the basic steps, even as a reviewer, and had trouble in several instance to get the code working. In the basic example, for instance, the plane wave can't serve as the basis for a far-field. Also, I have the error UndefVarError:sphericalGridPointsnot defined.

I installed via conda-forge.

(I'll be "out of office" for a week now, sorry about the delay).

PS: there is a "puplications" typo in the article.

HoBeZwe commented 1 year ago

Hi, probably the problem here is that you are not using the newest version of the package (hence, the function sphericalGridPoints which was added during the review does not exist for you). The reason for this is that I have not released a new version so far and the package manager pulls the latest release not the most recent commit.

To run the latest "developer" version you can add the package other than described in the documentation as 'dev SphericalScattering' instead of 'add SphericalScattering'. Then all new features should be available.

To avoid such misunderstandings I updated the readme to clearly point to the dev and the stable documentation (also via badges).

Basically, I would be surprised to see errors occur since, e.g., all of the Application Examples are actually executed on GitHub when creating the Documentation. That is, the results in the documentation are obtained by actually running the code.

That the far-field of a plane wave is not defined is intentional as noted in the documentation.

HoBeZwe commented 1 year ago

I've also corrected the typo. Thanks for pointing it out.

HoBeZwe commented 12 months ago

Hi @pdebuyl, could my suggestions resolve the problems with running the code?

pdebuyl commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

pdebuyl commented 11 months ago

@editorialbot check references

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

OK DOIs

- 10.1017/CBO9780511574498 is OK
- 10.1109/TAP.2023.3247896 is OK
- 10.1109/TAP.2023.3234704 is OK
- 10.23919/EuCAP57121.2023.10133154 is OK
- 10.1109/APS/URSI47566.2021.9703799 is OK
- 10.1109/AP-S/USNC-URSI47032.2022.9886833 is OK
- 10.23919/EuCAP53622.2022.9769650 is OK
- 10.1109/USNC-URSI52151.2023.10238214 is OK
- 10.1109/OJAP.2021.3121097 is OK
- 10.1137/141000671 is OK
- 10.21105/joss.00691 is OK
- 10.5281/zenodo.7949263 is OK
- 10.5281/zenodo.248729 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.5281/zenodo.1213225 is OK
- 10.5281/zenodo.5703291 is OK
- 10.1016/j.jqsrt.2017.05.010 is OK
- 10.1364/OSAC.399979 is OK

MISSING DOIs

- None

INVALID DOIs

- None
pdebuyl commented 11 months ago

Hello @HoBeZwe

It took me some time to get your code working. With the "dev install", most worked well, thanks for the help.

PlotlyJS on the other hand refused to work. After some googling, it appeared that this library does not support running from a conda install. Even though conda is not an "official end-to-all solutions" it is a convenient one for testing software and the fact that this does not work should be documented.

For information, as you have github repositories in your references, I made sure that they were all save on @softwareheritage to avoid the possible loss of dependencies.

So, my only request is that the incompatibility of the plotting library with conda be stated in the documentation.

Once this is done, please make an archive on zenodo and let me know here of the DOI and version name that you used there. The zenodo entry's authorship and title must match the article.

HoBeZwe commented 11 months ago

Sorry to hear about the inconvenience. Indeed, after a quick test via conda I also got an error when trying to use PlotlyJS.

I added a corresponding warning to the documentation: https://hobezwe.github.io/SphericalScattering.jl/dev/manual/#Plotting-Fields

pdebuyl commented 11 months ago

@editorialbot set v0.6.0 as version

editorialbot commented 11 months ago

Done! version is now v0.6.0

pdebuyl commented 11 months ago

@editorialbot set 10.5281/zenodo.10211642 as archive

editorialbot commented 11 months ago

Done! archive is now 10.5281/zenodo.10211642

pdebuyl commented 11 months ago

@editorialbot recommend-accept

editorialbot commented 11 months ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 11 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/CBO9780511574498 is OK
- 10.1109/TAP.2023.3247896 is OK
- 10.1109/TAP.2023.3234704 is OK
- 10.23919/EuCAP57121.2023.10133154 is OK
- 10.1109/APS/URSI47566.2021.9703799 is OK
- 10.1109/AP-S/USNC-URSI47032.2022.9886833 is OK
- 10.23919/EuCAP53622.2022.9769650 is OK
- 10.1109/USNC-URSI52151.2023.10238214 is OK
- 10.1109/OJAP.2021.3121097 is OK
- 10.1137/141000671 is OK
- 10.21105/joss.00691 is OK
- 10.5281/zenodo.7949263 is OK
- 10.5281/zenodo.248729 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.5281/zenodo.1213225 is OK
- 10.5281/zenodo.5703291 is OK
- 10.1016/j.jqsrt.2017.05.010 is OK
- 10.1364/OSAC.399979 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 11 months ago

:wave: @openjournals/pe-eics, this paper is ready to be accepted and published.

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/4804, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

pdebuyl commented 11 months ago

Thanks @HoBeZwe sorry again for the delays in the editorial process.

pdebuyl commented 11 months ago

and consider proofreading the final proof linked above :-)