openjournals / joss-reviews

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

[REVIEW]: OptiCommPy: Open-source Simulation of Fiber Optic Communications with Python #6600

Closed editorialbot closed 5 months ago

editorialbot commented 7 months ago

Submitting author: !--author-handle-->@edsonportosilva<!--end-author-handle-- (Edson Porto da Silva) Repository: https://github.com/edsonportosilva/OptiCommPy/ Branch with paper.md (empty if default branch): add-paper Version: v0.9.0-alpha Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @klb2, @ebezzam, @taladjidi Archive: 10.5281/zenodo.11450597

Status

status

Status badge code:

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

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

@klb2 & @ebezzam & @taladjidi, 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 @lucydot 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 @klb2

📝 Checklist for @taladjidi

📝 Checklist for @ebezzam

editorialbot commented 7 months 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 7 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.23 s (1083.0 files/s, 313349.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            86           9888             86          15254
JavaScript                      13           2439           2514           9272
Python                          33           2374           3718           5227
Jupyter Notebook                19              0          13819           2699
SVG                              1              0              0           2671
CSS                              4            190             40            779
reStructuredText                86            220            237            211
TeX                              1             14              0            143
Markdown                         2             38              0            103
YAML                             2              6             20             28
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                           249          15181          20442          36422
-------------------------------------------------------------------------------

Commit count by author:

  1222  Edson Porto da Silva
    28  Adolfo Herbster
    28  edsonportosilva
     3  Sourcery AI
     2  Carlos Daniel Fontes da Silva
     1  daniel7fontes
editorialbot commented 7 months ago

Paper file info:

📄 Wordcount for paper.md is 958

✅ The paper includes a Statement of need section

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

OK DOIs

- 10.1109/50.622902 is OK
- 10.1109/JSTQE.2010.2044751 is OK
- 10.1109/JLT.2009.2039464 is OK
- 10.1109/JLT.2020.2996188 is OK
- 10.1109/JLT.2017.2786351 is OK
- 10.1109/JLT.2017.2662082 is OK

MISSING DOIs

- No DOI given, and none found for title: Fiber-Optic Communication Systems
- 10.1016/b978-1-78548-037-9.50001-6 may be a valid DOI for title: Digital Communications
- No DOI given, and none found for title: Robochameleon: A matlab coding framework and compo...
- No DOI given, and none found for title: OptiSystem
- No DOI given, and none found for title: VPItransmissionMaker™ Optical Systems 
- No DOI given, and none found for title: Synopsys OptSim for Optical Communication
- No DOI given, and none found for title: Optilux, the optical simulatr toolbox.
- No DOI given, and none found for title: CuPy: A NumPy-Compatible Library for NVIDIA GPU Ca...
- No DOI given, and none found for title: CUDA, release: 10.2.89

INVALID DOIs

- None
editorialbot commented 7 months ago

License info:

🟡 License found: GNU General Public License v3.0 (Check here for OSI approval)

editorialbot commented 7 months ago

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

klb2 commented 7 months ago

Review checklist for @klb2

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

taladjidi commented 7 months ago

Review checklist for @taladjidi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ebezzam commented 6 months ago

Review checklist for @ebezzam

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ebezzam commented 6 months ago

@lucydot, @edsonportosilva below is my initial review. I realize that there is an examples folder, but is it expected to run everything or is there a notebook/script that gives an overview of the main features?

Functionality

Documentation

Software paper

Miscellaneous

lucydot commented 6 months ago

Thank you for your thoughtful review @ebezzam

I realize that there is an examples folder, but is it expected to run everything or is there a notebook/script that gives an overview of the main features?

With the immediate review process in mind, this seems like an important thing to address. Could you indicate which examples demonstrate the key software functionality @edsonportosilva? This could be useful for future users, so I suggest it is also included in the documentation.

edsonportosilva commented 6 months ago

Hello @lucydot @ebezzam,

Thanks for the review and the feedback! In the examples folder, we provide notebooks with simulation setups with various levels of complexity to help the user. The simplest simulation of optical transmission (kind of a 'textbook example') is provided in basic_OOK_transmission.ipynb. This notebook would be the starting point e.g. for undergrad students who are taking a first course in optical communications.

The notebook test_WDM_transmission.ipynb is a more advanced example that deals with coherent optical transmission and uses main features of OptiCommPy (coherent communications, advanced fiber optic modeling, and digital signal processing (DSP)). It is also the kind of simulation targeted by experienced users, such as researchers/engineers working with optical communications. This notebook also automatically calls for GPU processing, if a suitable GPU is available. Thus, by simply running it in e.g. Colab with and without a GPU accelerator, the user can estimate how fast a GPU speeds up the simulation (for this kind of simulation, the difference is quite significant, at least 10x faster I would expect).

The other notebooks are mostly simulation scenarios to "test" various features of the code. In particular, we have included the notebook test_metrics.ipynb where all relevant functions that calculate performance metrics are tested and the results compared with results from communications theory. We are working on setting up proper automated tests for the code in the repository, but it is working in progress.

@ebezzam @lucydot, I will proceed in fixing the issues listed in your preliminary review.

lucydot commented 6 months ago

@edsonportosilva - thanks for the update.

As @ebezzam has already mentioned, it would be useful to have documentation to show / walk through the different features as it is difficult to know where to start with the examples folder. Often I see this as a (series of) notebooks displayed in ReadTheDocs. For example, a "Getting Started" code walkthrough showing the basic functionality. You could then link to the examples folder for the more advanced topics. This could also go some way to ensuring that the "Testing" JOSS criteria is met. We strongly encourage this would be a suite of automated tests, however the following can also be deemed acceptable:

Documented manual steps that can be followed to objectively check the expected functionality of the software (e.g., a sample input file to assert behaviour)

From here.

taladjidi commented 6 months ago

@lucydot @ebezzam

Sorry for the late reply, I was in leave. Below are my first comments.

Functionality

Documentation

Software paper

Overall it's a very nice project, while the examples need a bit of documentation work, they do seem to cover most of the functionalities of the package.

Best,

klb2 commented 6 months ago

@lucydot @edsonportosilva

I have now completed my review. I tried to limit my comments to new ones that have not been covered in the reviews above.

In general, the package is interesting and contains a lot of features. The main issues are related to the tests and documentation, which lacks significant information (even missing whole modules).

Documentation

Tests

Examples

Paper (Minor Stuff)

edsonportosilva commented 6 months ago

Hello @lucydot @ebezzam. Sorry for my late reply. I am currently overloaded with the end of semester tasks. @ebezzam, thanks again for the time reviewing this project. Here is the reply to your queries.

@lucydot, @edsonportosilva below is my initial review. I realize that there is an examples folder, but is it expected to run everything or is there a notebook/script that gives an overview of the main features?

Functionality

  • Installation: I would recommend putting the same installation in your README and in the documentation. You can create a README.rst file, and use that within your documentation (like this instead of creating a separate file so you don’t have to edit twice). I say this because I looked at the documentation for the installation steps; and python setup.py install didn’t work for me, but pip install . did (which was in your README).
  1. Fixed. Now the documentation page should be updated as @ebezzam suggested. Moreover, we removed python setup.py install which is deprecated.
  • Functionality: it would be useful to have a section or notebook to show the different features, as I’m not sure where I would start to try things out. I realize there’s an examples folder, but it is not clear where to start.
  1. Fixed. We have added to the documentation page a getting started , as suggested by @lucydot, notebook where the reader can follow all the steps to set up a basic simulation and visualize performance results. This notebook also reproduces the plots shown in the JOSS paper. Another notebook was included with an advanced example, but I am still working to finish it.
  • Performance: it would be interesting to quantify the speed-up thanks to GPU acceleration, as mentioned in Lines 61-64 of the paper.
  1. We have added to the examples folder a notebook with a few basic benchmark results of CPU vs GPU processing time for simulations involving split-step Fourier algorithms, which are the most demanding processing tasks that could be performed in simulations . A mention of those results was included in the paper.

Documentation

  1. Fixed by 1.
  • Automated tests: there are none nor manual steps to verify functionality.
  1. Partially fixed. We have included a few automated tests. However, the scope of OptiCommPy (communications, optics, DSP) makes it difficult to set up meaningful unitary tests for all functions. For example, there are several algorithms of equalization and carrier recovery implemented, and each has particular requirements for correct operation, which depends on the specific scenario where they are tested. For those working in the field, this is well understood, though. Our first idea to workaround this issue was to add notebooks in the examples folder to work as a "systemic test" for the relevant functions, such that the user could see cases of use of a given function and then verify that the code was working properly. In practice, most of the code in OptiCommPy is validated with data from optical experiments, i.e. we have been able to use OptiCommPy to process data obtained from fiber optic transmission experiments and then verify that the code (e.g. physical models for the fiber, DSP algorithms, etc) work with real data. We could try to upload a few experimental traces to the repo and add a notebook to demonstrate real data processing, but the files to be uploaded may be large, and the experimental scenarios will not cover all functions. In any case, our plan is to include in the future as many automated tests as possible.
  • Community guidelines: there is in the README, but as mentioned above for “Installation”, it could be nice to use the README in your documentation, as I didn’t see the contribution guidelines at first.
  1. Fixed by 1.

    Software paper

    • A statement of need: The “Statement of need” starts great by giving the relevant context, but I feel like there’s a paragraph missing after Line 40 to explicitly say something like “OptiCommPy addresses this need by…” and who the target audience is (similar to what’s said in “Summary”).
  2. Fixed

  • Quality of writing: I think Lines 41 and onward should be in a separate section, e.g. with the title “OptiCommPy Code Structure”. And another section on what example scripts/code for the different features could be useful.
  1. Fixed.

Miscellaneous

  1. Fixed.
edsonportosilva commented 6 months ago

@taladjidi, thanks for your time reviewing this project. Please, see below the replies to the points you listed.

@lucydot @ebezzam

Sorry for the late reply, I was in leave. Below are my first comments.

Functionality

  • Installation went without trouble, though I only tried through pip install .
  • Examples: some of the notebooks do not run (while I did not test them all, basic_EDFA.ipynb seems to use an old package structure and does not run. I ran this first since I use an EDFA in the lab and it was the first example in the directory.
  1. Fixed. The examples folder has been updated with some unused files removed and all the remaining notebooks should be running now without issues. Let me know if there is still something broken.
  • Performance: I agree with @ebezzam that benchmarks curves would be a welcome addition. This could maybe be one of the tests ?
  1. Fixed, partially. We have included a notebook in the examples folder with a benchmark for the most critical GPU processing tasks that may be required in the simulations available in the repository, but not covering the whole package. However, we are not claiming any particular performance gain when compared to any other simulator. We have just stated that GPU processing may speedup the simulations when compared to the code without it, and the results in the benchmark in the examples folder now provides evidence that supports this claim.

Furthermore, you will gain at least a factor of 2 performance on your FFT's if you explicitely define and cache the FFT plans (see get_fft_plan or directly through cupy.cuda.cufft) instead of using the cupyx.scipy.fft API.

  • Performance / FFT: I would advise switching to a more performant FFT library than Numpy's since most of you DSP will rely on it. There are a lot of potential candidates, pyFFTW could be a drop in replacement. In the same vein, a lot of operations could be parallelized and JIT compiled using Numba.
  1. Thanks for bringing up this point. Indeed, we have spent some time trying to optimize the performance of the split-step algorithms running with CuPy. I believe currently the implementation of FFT plans is done automatically by cupyx.scipy.fft. If you have any particular insights on how to further optimize performance, I would be glad to hear more.

Documentation

  • Examples: Jupyter notebooks are a great way to introduce the various functionalities of your software. I found that the graphical outputs were very clear. Maybe it would be good to add Markdown blocks in between the code blocks to link the code with the physical models in use (how does one equation translate in terms of your package functionalities).
  1. Thanks for the observation. Yes, that is our idea in the long run. However, what we observe is that this is not a big issue for the public that has an interest in using OptiCommPy, which is usually someone with a little bit of experience in the field. As you noted, the graphical outputs are sometimes more important for them. However, since we and other colleagues abroad are using OptiCommPy when teaching courses, having more information on the examples is more important.
  • Physical models: I would expect the docstrings of the models to cite some relevant paper (when needed) explaining the model more in detail, or one or two latex equations to succintly define the used conventions etc ...
  1. Fixed. We have included in the docstrings in most parts of the code citations to papers/books where one can find physical models, algorithms, equations, etc. described in detail.
  • In the paper, you compare to commercial / proprietary solutions, maybe you could include a section of your Readme to explain what funcitonalities are supported, or some kind of migration guide ? In a similar fashion, maybe you could include some performance comparisons.
  1. Indeed, that would be interesting. However, those commercial simulators are expensive and we do not have the licenses necessary to make a comparison with OptiCommPy. I didn't get the part of the "migration guide" that you have mentioned.

Software paper

  • The manuscript overall is well written and has a very good introduction.
  • It would be better to split the Statement of need and the functionalities that you introduce, in order to highlight the problem on one side and its solution on the other.
  1. Fixed
  • The package structure is maybe superfluous in the paper, and maybe could be replaced with benchmarks / comparisons with proprietary software (if you have access to it) ?

Overall it's a very nice project, while the examples need a bit of documentation work, they do seem to cover most of the functionalities of the package.

Thanks! :)

Best,

edsonportosilva commented 6 months ago

Hello @lucydot @klb2 thanks for your time reviewing this project. Please, see below the replies to the points you listed.

@lucydot @edsonportosilva

I have now completed my review. I tried to limit my comments to new ones that have not been covered in the reviews above.

In general, the package is interesting and contains a lot of features. The main issues are related to the tests and documentation, which lacks significant information (even missing whole modules).

Documentation

  • It would be nice to add a simple example after the installation instructions to verify that the installation worked.

1 . Fixed, partially. The content in the getting started notebook available in the documentation should run without issues.

  • In general, the documentation does not show any usage examples.
  1. Fixed. We have added to the docs two examples, a basic and an advanced.
  • Required version of matplotlib in documentation (>=1.4.3) does not match that in requirements/setup file (>=3.7.0)
  1. Fixed
  • Pandas is not stated as a dependency in documentation
  1. Fixed
  • It should be mentioned which scripts need to be run to reproduce the figures from the paper.
  1. Fixed. The getting started notebook reproduces the plots from the paper.
  • The documentation of the fec module is missing in the documentation.
  1. Fixed. The FEC module has been removed because it was just unfinished work that has been discontinued.
  • Community guidelines, e.g., a CONTRIBUTING.md file, are missing
  1. Fixed. The documentation now provides guidelines for contributing to the code.

Tests

  • tests/test_dsp.py does not run with the unittest package
  1. Could you elaborate on that issue? I can see that all tests in the test folder run with pytest.
  • The setup script (setup.py) says that the nose module is required for testing, but it is not actually required to run the unit tests in tests/
  1. Fixed.
  • examples/test_*.py can not be run with Python (ipython is required). However, this is not mentioned anywhere.
  1. Fixed. The scripts '.py' in the examples folder where redundant copies of the notebooks made by jupytext and were removed to avoid confusion with actual .py tests files.
  • There only seems to be a small number of unit tests. The code coverage is very low.
  1. Yes, you are right. We are working towards increasing the coverage in the long run, but for reasons explained before this is not an easy task given the scope of the repository. However, the notebooks in the examples folder should be enough to verify the reproducibility and correctness for the main features of the repository.
  • The eye diagrams in examples/test_dsp_core_functions.ipynb are not useful (just a blue box).
  1. Fixed.
  • Error when running ipython examples/test_clockRecovery.py: ValueError: operands could not be broadcast together with shapes (44000,) (43968,) (Line 121)
  1. Redundant file removed.
  • Error when running ipython examples/test_fec.py: ModuleNotFoundError: No module named 'optic.modulation' (I think, it should be optic.comm.modulation)
  1. File removed.
  • Also, the optic.fec module does not exist. I could not find any LDPC functionality in the documentation.
  1. Fixed in 6.
  • Same issues of examples/test_fec.py also present in examples/test_fec.ipynb
  1. Fixed in 6.
  • Error when running ipython examples/test_NLC_withDBP_WDM_transmission.py: ImportError: cannot import name 'phaseNoise' from 'optic.models.channels
  1. Fixed.
  • Large deviations between theory and Monte Carlo in examples/test_metrics.ipynb for small SNRs. What is the reason behind this? (I have also increased the number of samples by a factor of 10, which did not yield any improvement.)
  1. Those results are correct because the performance comparison was between APSK and PSK, and since APSK tends to perform better than PSK, the curves make sense.
  • ImportError in examples/test_ofdm.py
  1. File removed.

Examples

  • ImportError in examples/basic_EDFA.py
  1. File removed.

Paper (Minor Stuff)

  • Acronym DSP is defined twice
  1. Fixed.
  • Non-breaking space ~ in "Fig.~3" is actually printed as a tilde.
  1. Fixed.
lucydot commented 6 months ago

Thank you very much @ebezzam @taladjidi @klb2 for your thorough and timely reviews 🌟

And excellent that you have addressed so many points in a short timeframe @edsonportosilva

@ebezzam @taladjidi @klb2 - please re-visit your checklist when you get a chance, and see if there are any outstanding issues blocking acceptance. Reading discussion, there may be some uncertainty around testing criteria - in which case it would be great to hear your thoughts and I can feed in with editorial viewpoint. The other key aspect was documentation.

klb2 commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

klb2 commented 6 months ago

Thank you for addressing my comments @edsonportosilva. I checked the code and documentation again and only some issues are remaining.

Regarding point 8 from the previous comments: In the original submission, it was not clear/documented that pytest would be required to run the unit tests. Only the unittest module was imported in two out of the three files. But I saw that this is fixed now.

edsonportosilva commented 6 months ago

Hello @klb2, thanks again for the feedback,

Thank you for addressing my comments @edsonportosilva. I checked the code and documentation again and only some issues are remaining.

  • The file Image("./figures/BasicIMDD.png", width=600) in the getting_started.ipynb notebook does not exist.
  1. I don't know the problem in this case, because for me the figure is displayed both in the repository and the documentation page. Does anyone else see the same issue? @lucydot @ebezzam @taladjidi
  • I am still not sure about the correctness of the results in the test_metrics.ipynb notebook.

    • First, for low SNRs, the Monte Carlo simulation results systematically deviate from the theoretical results (Sections 1.1 and 1.2). This should not be the case, especially for small error rates with enough samples. 2024-05-07-124244_837x519_scrot
  1. The deviation from Monte Carlo to theoretical results is expected in this case for large QAM/PSK constellations. This happens because the expression used to calculate the error probability assumes that the error events are dominated by errors between neighboring QAM/PSK symbols, in a gray-mapped constellation. That is, every symbol error translates into a bit error. However, this is only true in moderate to low symbol-error-rate (SER) regimes. If the SER is above 1e-1, this assumption is not true anymore and the Monte Carlo results should deviate from the approximated theoretical curves, which would then underestimate the BER. I have added an explanation to the function's (theoryBER) docstring to clarify this point. Nevertheless, such regimes of high error rates are mostly of no practical relevance for standard digital communication systems, including fiber optic communications.
  1. Thanks for reporting this issue. This deviation had not appeared before and I did a quick investigation to find that is related to the version of Numba. Results are fine with versions 0.54.0-0.57.0 and starting from version 0.58.0 this deviation appears. These results are obtained by integrating numerically the theoretical expression of the mutual information calculated between input and output for the memoryless Gaussian channel. It requires a double integral to be calculated with dblquad, whose argument is compiled with Numba to speed up the calculations. It turns out that different versions of Numba appear to be providing different precision for the calculation of this function, and some numerical instability is causing the results to deviate from the correct curves. Due to the lack of a better solution, for now, I have restricted the version of Numba in the requirements to 0.54.0-0.57.0 to work around this issue. See the results below.

image

  • I did not see this before, but it should be "Fig. 2" instead of "Fig. 3" in the paper (line 82 on page 3).
  1. Fixed.

Regarding point 8 from the previous comments: In the original submission, it was not clear/documented that pytest would be required to run the unit tests. Only the unittest module was imported in two out of the three files. But I saw that this is fixed now.

klb2 commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

lucydot commented 6 months ago

I don't know the problem in this case, because for me the figure is displayed both in the repository and the documentation page. Does anyone else see the same issue?

It is displayed here: https://github.com/edsonportosilva/OptiCommPy/blob/main/examples/getting_started.ipynb. But when I look in the figures folder, it is true the image does not exist. So I think if you run locally, it will give the reported error. The notebook being displayed online must have been executed before the image was removed. In fact, the path to the figures folder also seems to have changed.

@ebezzam @taladjidi - a reminder to re-visit your checklist when you get a chance, and see if there are any outstanding issues blocking acceptance.

taladjidi commented 6 months ago

Hi @lucydot , on my side it looks good ! Thank you @ebezzam for the swift reply, I will try to draft a PR for the FFT / kernel performance stuff I identified.

edsonportosilva commented 6 months ago

@lucydot @klb2, thanks! Found the problem. All should be fine now with the notebook and the figure is uploaded here.

I don't know the problem in this case, because for me the figure is displayed both in the repository and the documentation page. Does anyone else see the same issue?

It is displayed here: https://github.com/edsonportosilva/OptiCommPy/blob/main/examples/getting_started.ipynb. But when I look in the figures folder, it is true the image does not exist. So I think if you run locally, it will give the reported error. The notebook being displayed online must have been executed before the image was removed. In fact, the path to the figures folder also seems to have changed.

@ebezzam @taladjidi - a reminder to re-visit your checklist when you get a chance, and see if there are any outstanding issues blocking acceptance.

klb2 commented 6 months ago

Thanks for addressing my remaining comments @edsonportosilva.

One last suggestion is that you could add (and emphasize) on the main page of the documentation that the software is open-source.
However, I guess that is optional and I have completed my review @lucydot.

ebezzam commented 5 months ago

@edsonportosilva thank you for addressing my comments. The Getting started page with interleaved code and description looks very nice.

Just some minor stuff, but otherwise it looks good for me:

# Examples

In the documentation, one can find [an example](https://opticommpy.readthedocs.io/en/latest/getting_started.html) that demonstrates core features of OptiCommPy. A collection of examples to build several different simulation setups, including advanced setups with non-linear fiber propagation models, WDM transmission, and coherent detection can be found in the repository's [examples](https://github.com/edsonportosilva/OptiCommPy/tree/main/examples) folder. [Benchmarks](https://github.com/edsonportosilva/OptiCommPy/blob/main/examples/benchmarck_GPU_processing.ipynb) quantifying the speedup achieved by using GPU acceleration are also provided.

which renders as

Examples

In the documentation, one can find an example that demonstrates core features of OptiCommPy. A collection of examples to build several different simulation setups, including advanced setups with non-linear fiber propagation models, WDM transmission, and coherent detection can be found in the repository's examples folder. Benchmarks quantifying the speedup achieved by using GPU acceleration are also provided.

lucydot commented 5 months ago

Great work @ebezzam @taladjidi @klb2 - thanks for your thoughtful and timely reviews.

@edsonportosilva it looks like there are a few relatively minor points to address above, and then we can progress to next stage (final check throughs by editor and author).

edsonportosilva commented 5 months ago

Thanks for the feedback, @ebezzam @lucydot!

@edsonportosilva thank you for addressing my comments. The Getting started page with interleaved code and description looks very nice.

Thanks :)

Just some minor stuff, but otherwise it looks good for me:

  • The "Open in Colab" button doesn't work for me. From ReadTheDocs, it is not clickable and from GitHub I get the error Notebook not found: Could not find JOSS_paper_example.ipynb in https://api.github.com/repos/edsonportosilva/OptiCommPy/contents/examples?
  1. Should be fixed by now in the GitHub notebook. On the documentation page, for some reason, the logo is not clickable, and I noticed that happening for a few other badges. I assume this is related to some bug in the translation of the notebook content to an RST document. I will try to find a solution for that but, for now, the link in the repository should be enough to open the notebook in Colab.
  • In your paper, it could be nice to have an "Examples" section with links, e.g. adapting lines 83-87 as:
# Examples

In the documentation, one can find [an example](https://opticommpy.readthedocs.io/en/latest/getting_started.html) that demonstrates core features of OptiCommPy. A collection of examples to build several different simulation setups, including advanced setups with non-linear fiber propagation models, WDM transmission, and coherent detection can be found in the repository's [examples](https://github.com/edsonportosilva/OptiCommPy/tree/main/examples) folder. [Benchmarks](https://github.com/edsonportosilva/OptiCommPy/blob/main/examples/benchmarck_GPU_processing.ipynb) quantifying the speedup achieved by using GPU acceleration are also provided.

which renders as

Examples

In the documentation, one can find an example that demonstrates core features of OptiCommPy. A collection of examples to build several different simulation setups, including advanced setups with non-linear fiber propagation models, WDM transmission, and coherent detection can be found in the repository's examples folder. Benchmarks quantifying the speedup achieved by using GPU acceleration are also provided.

  1. Thanks for the suggestion and we have changed the text accordingly.
edsonportosilva commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

lucydot commented 5 months ago

Wonderful! We're onto the last steps @edsonportosilva.

Thank you for your reviews @ebezzam @taladjidi @klb2 ✨

I'll ask editorialbot to generate the post-review checklist; let me know when each author item is done.

lucydot commented 5 months ago

@editorialbot generate post-review checklist

lucydot commented 5 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

lucydot commented 5 months ago

@edsonportosilva, please adjust the paper so that when you have multiple citations are contained in the same bracket. This can be done using semi-colons to separate items (see here). After you have adjusted the paper, please re-compile using @editorialbot generate pdf to check all are formatted as expected.

edsonportosilva commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

edsonportosilva commented 5 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Hello, @lucydot! Sorry for the late reply, last two weeks I have been unable to proceed with the final steps of the review. Here it is:

  • Double check authors and affiliations (including ORCIDs)
  1. Done
  • Make a release of the software with the latest changes from the review and post the version number here. This is the version that will be used in the JOSS paper.
  1. https://github.com/edsonportosilva/OptiCommPy/releases/tag/v0.9.0-alpha
  • Archive the release on Zenodo/figshare/etc and post the DOI here.
  1. https://doi.org/10.5281/zenodo.11450597
  • Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.
  1. Done.
  • Make sure that the license listed for the archive is the same as the software license.
  1. Done
edsonportosilva commented 5 months ago

@edsonportosilva, please adjust the paper so that when you have multiple citations are contained in the same bracket. This can be done using semi-colons to separate items (see here). After you have adjusted the paper, please re-compile using @editorialbot generate pdf to check all are formatted as expected.

Done

lucydot commented 5 months ago

@editorialbot set 10.5281/zenodo.11450597 as archive

editorialbot commented 5 months ago

Done! archive is now 10.5281/zenodo.11450597

lucydot commented 5 months ago

@editorialbot set v0.9.0-alpha as version

editorialbot commented 5 months ago

Done! version is now v0.9.0-alpha

lucydot commented 5 months ago

@editorialbot check references

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

OK DOIs

- 10.1109/50.622902 is OK
- 10.1109/JSTQE.2010.2044751 is OK
- 10.1109/JLT.2009.2039464 is OK
- 10.1109/JLT.2020.2996188 is OK
- 10.1109/JLT.2017.2786351 is OK
- 10.1109/JLT.2017.2662082 is OK

MISSING DOIs

- No DOI given, and none found for title: Fiber-Optic Communication Systems
- No DOI given, and none found for title: Digital Communications
- No DOI given, and none found for title: Robochameleon: A matlab coding framework and compo...
- No DOI given, and none found for title: OptiSystem
- No DOI given, and none found for title: VPItransmissionMaker™ Optical Systems 
- No DOI given, and none found for title: Synopsys OptSim for Optical Communication
- No DOI given, and none found for title: Optilux, the optical simulatr toolbox.
- No DOI given, and none found for title: CuPy: A NumPy-Compatible Library for NVIDIA GPU Ca...
- No DOI given, and none found for title: CUDA, release: 10.2.89

INVALID DOIs

- None
lucydot commented 5 months ago

Hi @edsonportosilva, can you check and confirm that there are no DOIs available for the references above? I can see they are either software or book references, so less likely to be available than for journal references.

After that I will recommend acceptance to the track editors 🥳

edsonportosilva commented 5 months ago

Hi @lucydot,

Hi @edsonportosilva, can you check and confirm that there are no DOIs available for the references above? I can see they are either software or book references, so less likely to be available than for journal references.

I have checked and there is no DOI available for those references, mostly because they are either software or physical books, as you noted.

After that I will recommend acceptance to the track editors 🥳

Great to hear that and thank you for your guidance during this peer review process 😊