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]: ClassipyGRB: Machine Learning-Based Classification and Visualization of Gamma Ray Bursts using t-SNE #5923

Closed editorialbot closed 7 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@KenethGarcia<!--end-author-handle-- (Keneth Stiven Garcia Cifuentes) Repository: https://github.com/KenethGarcia/ClassiPyGRB Branch with paper.md (empty if default branch): Version: v1.1.0 Editor: !--editor-->@dfm<!--end-editor-- Reviewers: @wkerzendorf, @dfm Archive: 10.5281/zenodo.10909942

Status

status

Status badge code:

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

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

@wkerzendorf & @dfm, 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 @dfm 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 @dfm

📝 Checklist for @wkerzendorf

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.26 s (91.8 files/s, 42337.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Jupyter Notebook                11              0           4354           3625
Python                           8            288            682           1751
Markdown                         2             54              0            122
TeX                              1              5              0             98
TOML                             1              5              0             61
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                            24            353           5040           5675
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3847/1538-4357/acb999 is OK
- 10.1086/186969 is OK
- 10.1038/s41586-022-05403-8 is OK
- 10.3847/2041-8213/ab964d is OK
- 10.48550/arXiv.2304.08666 is OK
- 10.48550/arXiv.2201.05145 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

Wordcount for paper.md is 891

dfm commented 1 year ago

@wkerzendorf — This is the review thread for the paper. All of our correspondence will happen here from now on. Thanks again for agreeing to participate!

👉 Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting @editorialbot generate my checklist on this issue ASAP. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#5923 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for the review process to be completed within about 4-6 weeks but please try to make a start ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. Please get your review started as soon as possible!

dfm commented 1 year ago

Review checklist for @dfm

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

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:

dfm commented 1 year ago

@wkerzendorf — This is a ping to keep this on your radar. Please generate your checklist and start going through it ASAP! Thanks!!

wkerzendorf commented 1 year ago

Review checklist for @wkerzendorf

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

wkerzendorf commented 1 year ago

Functionality

Installing the software worked but subsequently running the examples did not:

---------------------------------------------------------------------------
FileNotFoundError                         Traceback (most recent call last)
Cell In[1], line 3
      1 from ClassiPyGRB import SWIFT
      2 swift = SWIFT(res=64)
----> 3 df = swift.obtain_data(name='GRB211211A')

File ~/miniconda/envs/joss_classipygrb/lib/python3.8/site-packages/ClassiPyGRB/_swift.py:174, in SWIFT.obtain_data(self, name, check_disk)
    172         raise RuntimeError(f"Error from tables when trying to read: {e}. Try to re-install tables package.")
    173 else:
--> 174     with resources.open_text(summary_tables, 'summary_general.txt') as file:
    175         grb_names, ids = np.genfromtxt(file, delimiter="|", dtype=str, usecols=(0, 1), unpack=True,
    176                                        autostrip=True)
    177     if len(grb_names) == 0 or not isinstance(grb_names, (Sequence, Mapping, np.ndarray)):

File ~/miniconda/envs/joss_classipygrb/lib/python3.8/importlib/resources.py:126, in open_text(package, resource, encoding, errors)
    124 if reader is not None:
    125     return TextIOWrapper(reader.open_resource(resource), encoding, errors)
--> 126 _check_location(package)
    127 absolute_package_path = os.path.abspath(package.__spec__.origin)
    128 package_path = os.path.dirname(absolute_package_path)

File ~/miniconda/envs/joss_classipygrb/lib/python3.8/importlib/resources.py:82, in _check_location(package)
     80 def _check_location(package):
     81     if package.__spec__.origin is None or not package.__spec__.has_location:
---> 82         raise FileNotFoundError(f'Package has no location {package!r}')

FileNotFoundError: Package has no location <module 'ClassiPyGRB.summary_tables' (namespace)>

There are also no automated ways to run the tests or descriptions of how to run tests. There are also no CI/CD workflows for this repository.

The repository has a rudimentary contribution guidelines but without testing frameworks it might be hard to check if contributions break existing code.

Documentation The installation instructions are sparse and there is conflicting information in the paper compared to the main branch. The installation works as described using pip install . but it would be nice to give a bit more guidance for users on other systems (macos/windows/other distros. Perhaps giving a conda environment might be helpful.

The Documentation also only exists of a single Readme.md without a proper documentation page that would give more insights on API and usage.

Paper

The paper has several typos and I would suggest re-reading it carefully and employing a grammar checker.

The paper does not make a complete State of the Field analysis but claims that have not previously been identified by other groups without giving specific references.

The paper also often uses qualitative words such as easy and fast without quantitative description of what makes it easy and or fast (these are just some examples).

dfm commented 11 months ago

@KenethGarcia — I wanted to check in to see if you've had an opportunity to look at any of the issues I opened or @wkerzendorf's comments above? It's useful to iteratively address comments as the review progresses. Let us know when you'll have a chance to work on this!

KenethGarcia commented 11 months ago

@dfm Thank you for reaching out! We appreciate your proactive approach to the ongoing review. Actually, we are addressing the issues you've opened, as well as carefully considering @wkerzendorf comments. We expect to answer the issues in the next days for further comments. Actually, there are minor updates in the GitHub repository, and we are implementing a conda installation with an updated catalogue of GRBs (up to 2023).

KenethGarcia commented 9 months ago

Dear @dfm and @wkerzendorf,

We are writing to leave you with some updates on changes that have been made to ClassiPyGRB based on your comments and feedback:

Regarding the comment about “Installation and cross-platform support”:

We updated the installation tutorial by adding conda/mamba installation instructions. We conducted several hours of testing and environment validation to ensure a seamless and reliable installation process. Furthermore, we also remark that in previously installed tkinter/pytables OS, the requirements of creating the conda environment are unnecessary.

About testing and CI workflows:

We have implemented the following documentation for Test Running Procedure:

  1. We have added a documentation section in the repository's README, explaining how to run the tests.
  2. A GitHub Actions workflow has been created to automate the testing process. This workflow is triggered on each push to the repository, and it ensures that the tests are executed in a controlled and consistent environment. You can find the workflow configuration file in the .github/workflows directory.

About the Statement of Need:

We added a new section where we specify:

Thank you very much for your comments, we will be looking forward to any additional suggestions to this library.

Best Regards

dfm commented 9 months ago

@KenethGarcia — Thanks for your work on this. I've now finished going through my checklist and opened some last small issues and one PR. Please take a look at those! After those are finished I'm happy to recommend acceptance.

dfm commented 9 months ago

@wkerzendorf — Can you also take a look at this again soon and try to go through your last checklist items? Thanks!!

KenethGarcia commented 9 months ago

Thank you @dfm for your comments!

We will work around the current documentation and additional comments that you have submitted.

Best Regards!

dfm commented 8 months ago

@wkerzendorf — Pinging you to keep this on your radar! Please try to get back to this ASAP. Many thanks!

wkerzendorf commented 8 months ago

Sorry all - didn't see the ping from this. Will look at this in the next two weeks if not sooner.

wkerzendorf commented 8 months ago

Can you generate new proofs?

dfm commented 8 months ago

@editorialbot generate pdf

dfm commented 8 months ago

@wkerzendorf thanks! You should be able to ask the bot for a new pdf too in the future!

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:

wkerzendorf commented 8 months ago

The State of the Field in the paper is missing. Also there are references to a requeriments.txt in the paper doesn't seem to exist in the code.

KenethGarcia commented 8 months ago

Dear @wkerzendorf,

We have corrected the references to a requirements.txt of ClassiPyGRB. In the new version, we specify that all the necessary packages will be automatically handled during installation.

On the other hand, we have updated the Statement of Need section, adding a more accurate description of what problems the software is designed to solve, who the target audience is, and the differences with other similar packages.

Thank you very much for your comments again, we will be looking forward to any additional suggestions to this package.

Best regards

KenethGarcia commented 8 months ago

@editorialbot generate pdf

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:

KenethGarcia commented 8 months ago

@editorialbot generate pdf

(A minor update)

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:

KenethGarcia commented 7 months ago

Hi @wkerzendorf,

Just a quick follow-up regarding the previous feedback:

Was the addition of the State of the Field section and the adjustments to the references satisfactory?

Thank you!

Best regards,

Keneth

KenethGarcia commented 7 months ago

Dear @dfm ,

I hope this message finds you well. I am writing to seek your guidance and assistance regarding the recent feedback we received on ClassiPyGRB.

As per the suggestions provided by @wkerzendorf , we have made the corrections required, including addressing the missing State of the Field section and rectifying the references to a requirements.txt file. However, there has been no confirmation on whether these adjustments meet the JOSS requirements. Can u help us?

Thank you for your attention to this matter, and I look forward to your prompt response.

Best regards,

Keneth On behalf of the authors

wkerzendorf commented 7 months ago

@dfm looks good from my side.

dfm commented 7 months ago

@wkerzendorf — Thanks! Can you check the rest of your boxes on the checklist above ☝️ or comment explicitly that you're happy with the unchecked boxes?

@KenethGarcia — Thanks for your patience! In the next day or two I'll work through the final editorial steps and have a few more things I need from you.

dfm commented 7 months ago

@editorialbot check references

dfm commented 7 months ago

@editorialbot generate pdf

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

OK DOIs

- 10.3847/1538-4357/acb999 is OK
- 10.1086/186969 is OK
- 10.1038/s41586-022-05403-8 is OK
- 10.3847/2041-8213/ab964d is OK
- 10.48550/arXiv.2304.08666 is OK
- 10.48550/arXiv.2201.05145 is OK
- 10.1093/mnras/stad3624 is OK
- 10.3847/1538-4357/ace325 is OK
- 10.3847/1538-4357/ac9d38 is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

dfm commented 7 months ago

@KenethGarcia — I've gone through the manuscript and it all looks good to me. I expect @wkerzendorf will formally sign off soon.

In the meantime, can you go through these final steps:

  1. Take one last read through the manuscript to make sure that you're happy with it (it's harder to make changes later!), especially the author names and affiliations. I've taken a pass and it looks good to me!
  2. Increment the version number of the software and report that version number back here.
  3. Create an archived release of that version of the software (using Zenodo or something similar). Please make sure that the metadata (title and author list) exactly match the paper. Then report the DOI of the release back to this thread.
KenethGarcia commented 7 months ago

Dear @dfm,

As you request, we have added:

  1. A new version in the GitHub repository of ClassiPyGRB, with the label v1.1.0
  2. We have reviewed the paper again looking for updates, and we have made minor changes in the affiliations and acknowledgments sections.
  3. We have updated an archived version of the software in Zenodo, and it has assigned the DOI 10.5281/zenodo.10909942. Moreover, we also have carefully checked the title and authors.

Thank you for the indications and we will stay in contact,

Best regards,

Keneth On behalf of the authors

@editorialbot generate pdf

KenethGarcia commented 7 months ago

@editorialbot generate pdf

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:

dfm commented 7 months ago

@editorialbot set v1.1.0 as version

editorialbot commented 7 months ago

Done! version is now v1.1.0

dfm commented 7 months ago

@editorialbot set 10.5281/zenodo.10909942 as archive

editorialbot commented 7 months ago

Done! archive is now 10.5281/zenodo.10909942

dfm commented 7 months ago

@KenethGarcia — Thank you!! Everything is looking good over here. I'm still waiting on a final word from @wkerzendorf, since the checklist isn't fully completed. I've sent several emails as well - hoping we hear back soon!

wkerzendorf commented 7 months ago

All looks good. I'm happy to accept.

dfm commented 7 months ago

Thanks @wkerzendorf!!

dfm commented 7 months ago

@editorialbot check references

dfm commented 7 months ago

@editorialbot generate pdf

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

OK DOIs

- 10.3847/1538-4357/acb999 is OK
- 10.1086/186969 is OK
- 10.1038/s41586-022-05403-8 is OK
- 10.3847/2041-8213/ab964d is OK
- 10.48550/arXiv.2304.08666 is OK
- 10.48550/arXiv.2201.05145 is OK
- 10.1093/mnras/stad3624 is OK
- 10.3847/1538-4357/ace325 is OK
- 10.3847/1538-4357/ac9d38 is OK

MISSING DOIs

- None

INVALID DOIs

- None