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

[REVIEW]: islatu: A Python package for the reduction of reflectometry data #4397

Closed
editorialbot opened this issue May 16, 2022 · 62 comments
Closed
Assignees
Labels
accepted Jupyter Notebook published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review TeX Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented May 16, 2022

Submitting author: @RBrearton (Richard Brearton)
Repository: https://github.com/RBrearton/islatu
Branch with paper.md (empty if default branch): islatu_paper
Version: 1.0.7
Editor: @jgostick
Reviewers: @andyfaff, @daguiam
Archive: 10.5281/zenodo.7105217

Status

status

Status badge code:

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

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

@andyfaff, 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 @jgostick 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 @andyfaff

📝 Checklist for @daguiam

@editorialbot
Copy link
Collaborator Author

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

Software report:

github.com/AlDanial/cloc v 1.88  T=0.01 s (520.8 files/s, 25521.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Markdown                         2             14              0            120
TeX                              1              3              0             36
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                             4             18              4            174
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

@editorialbot
Copy link
Collaborator Author

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

OK DOIs

- 10.5281/zenodo.6393464 is OK

MISSING DOIs

- 10.1107/s1600577516009875 may be a valid DOI for title: Diamond beamline I07: a beamline for surface and interface diffraction

INVALID DOIs

- None

@editorialbot
Copy link
Collaborator Author

Wordcount for paper.md is 1011

@editorialbot
Copy link
Collaborator Author

Failed to discover a valid open source license

@editorialbot
Copy link
Collaborator Author

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

@andyfaff
Copy link

andyfaff commented May 16, 2022

Review checklist for @andyfaff

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/RBrearton/islatu?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@RBrearton) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@andyfaff
Copy link

License: yes, but the setup.py file needs updating with the correct licence identifier.

Quality of writing: Good, but there are a few spelling mistakes that could be picked up by a spell checker.

State of the field: It might be worth mentioning a couple of other reduction packages in the arena, and how islatu works compared to them.

Community guidelines: there aren't any that I can discern.

Installation instructions: the instructions in README.md don't work out of the box, they should be updated. There is a requirements.txt file that works.

Installation: does not proceed according to the instructions.

References: It might be worthwhile including links to the articles cited?

@jgostick
Copy link

@editorialbot add @daguiam as reviewer

@editorialbot
Copy link
Collaborator Author

@daguiam added to the reviewers list!

@jgostick
Copy link

Hi @daguiam
You'll want to generate an 'editorial checklist' for yourself, so you can see which aspects of the codebase to inspect and approve. You can do this by typing @editorialbot generate my checklist into a comment box, then you'll have a list like @andyfaff's above. The review process typically takes at least a month, so if you can set aside an afternoon or two of the next few weeks, then that should be sufficient, at least for the first pass of reviews.

@daguiam
Copy link

daguiam commented May 23, 2022

Review checklist for @daguiam

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/RBrearton/islatu?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@RBrearton) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

@jgostick
Copy link

jgostick commented Jun 8, 2022

Hi @andyfaff and @daguiam, how are things coming along with this review? I assume you are in contact with the author via their github repo, so hopefully things are progressing?

@andyfaff
Copy link

andyfaff commented Jun 8, 2022

I've submitted a few issues, waiting for those to progress.

@RBrearton
Copy link

Ahh, my apologies! For some reason I missed the notification that issues had been raised in the github repository. I'll work through those over the next day or so and let you know when everything is finished on my end.

@jgostick
Copy link

Hi All, how are things? I've put this review on the back burner for the past few weeks, and judging by the progress here I'm not the only one! I hope things are moving along nicely behind the scenes?

@RBrearton
Copy link

Hi there, I responded to the issues that were raised on the repo, but I wasn't sure exactly how to proceed.

@andyfaff mentioned that I don't currently refer to any other papers in the field in the article, which is a good point and I'll change the text of the article accordingly. I didn't want to make any changes to the article until I know all of the changes that I'll need to make, so I was wondering if anyone else had any comments or if I should go ahead and make the suggested changes?

@jgostick
Copy link

I have just pinged @daguiam via email, so hopefully he'll be able to pick-up the process.

@daguiam
Copy link

daguiam commented Jul 1, 2022

Hi! I have continued the review since the last issues were resolved and I have some comments below. Overall, despite some bugs that I have added as issues, the package meets most of the checks.

Functionality:
The issues with the installation steps have been corrected and it is now straightforward to install the package.
Following through the provided notebooks the functionality appears to work as claimed in terms of reducing the reflectometry data with appropriate signal processing, error propagation and data correction.
The packaged has some hardcoded strings related to paths and where to find the datasets (with hardcoded network paths to the Diamond I believe) if the Profile is not provided the direct data folder path, assuming that the user is inside the network and has access to those directories. These could be removed or pass an argument flag to look for the datasets in those directories.

Documentation:
Example usage: The documentation provided is sufficient to follow the workflow of I07 reflectometry data reduction using the notebooks. The process_xrr.py CLI is not documented in the readthedocs.
Statement of need: The statement of need is better described in the paper than in the software documentation.
Missing community guidelines on how to contribute to the project or contact the authors.
The README.md could provide a direct link to the documentation website and the notebook that gives an example of the data reduction workflow.

Software paper
The software paper describes well the need for such a package and who the target audience is. It also presents the overall functionality of the package. It is lacking a comparison with other reflectometry data reduction packages or efforts made, as stated by @andyaff. The quality of the writing is good and reference to cited packages are provided.

@jgostick
Copy link

jgostick commented Jul 9, 2022

Thanks @daguiam for your review! @RBrearton, I think you have somethings to work on the keep you busy? I assume all is progressing well?

@RBrearton
Copy link

Sorry for being slow, I just got back from some annual leave. I'll work through the comments raised by @daguiam in the next few days (due to a change in the structure of some .nxs files produced by diamond I need to make some changes to islatu anyway, so now is a good time to remove hard coded paths and the like).

@RBrearton
Copy link

Hi @daguiam,

With regards to hardcoded paths, I think these are only given where appropriate at the moment. The only diamond filesystem path in the repo is in a default argument to i07reduce in runner.py, which seems like a reasonable default since that function is designed to specifically handle data from diamond's i07 beamline. Another local path can of course be passed in place of that default argument.

The CLI also often checks for a directory called 'processing', but these checks should all be harmless and no errors should be raised if the directory doesn't exist. I'm aware that there was such an error, but that has now been fixed!

The documentation has been updated to include contact details, a brief contributing section and a discussion (with examples) on how to use the command line interface. These examples use the diamond directory structure, but changing the data directory to be a local directory should be completely clear. I just chose this because the people most likely to come across the readthedocs page are diamond users, and this should make more sense to them.

I updated the readme.md to link directly to the docs, as suggested. I'll provide another update when I update the paper to include references/comparisons to other reflectivity reduction tools.

@jgostick
Copy link

jgostick commented Aug 3, 2022

Hi All, how are things progressing here? I just got back from my first "in person" conference followed by a week of vacay, so I haven't been keeping tabs on this. Anyway, I'm back now so let's drive this submission into the end zone!

@RBrearton
Copy link

Hi @jgostick, I just updated the paper to add a paragraph briefly discussing software packages that do similar things. I also fixed a couple of typos. As far as I'm aware, that's about it on my end?

@RBrearton
Copy link

Zenodo is still broken for automatically archiving repositories (at least on my end it is), but I did a manual upload so that we aren't just waiting forever.

I've updated the paper to use the new doi (10.5281/zenodo.7105217). I double checked affiliations and made a new release with all the changes (in fact, I made a lot of released with the changes while trying to get zenodo to work, so we ended on v1.0.7).

Let me know if anything else needs to be done!

@jgostick
Copy link

@editorialbot set 1.0.7 as version

@editorialbot
Copy link
Collaborator Author

Done! version is now 1.0.7

@jgostick
Copy link

@editorialbot set 10.5281/zenodo.7105217 as archive

@editorialbot
Copy link
Collaborator Author

Done! Archive is now 10.5281/zenodo.7105217

@jgostick
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@jgostick
Copy link

Hey @RBrearton, the PDF has 3 authors, but the zenodo archive only lists you and Andrew McCluskey

@RBrearton
Copy link

Is this a hard requirement? Tim supervised the project. He provided direction and helped both McCluskey and me a great deal. He didn't directly push any commits to the repository, though, so I thought it was appropriate to list him an author on the article but not as an author of the source code.

I can change this if required, but I thought this was a good way of representing authorship!

@jgostick
Copy link

I'm not actually sure. I think I need the EiC to answer this one.

@jgostick
Copy link

@editorialbot check references

@editorialbot
Copy link
Collaborator Author

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

OK DOIs

- 10.5281/zenodo.7105217 is OK

MISSING DOIs

- 10.1107/s1600577516009875 may be a valid DOI for title: Diamond beamline I07: a beamline for surface and interface diffraction

INVALID DOIs

- None

@danielskatz
Copy link

Is this a hard requirement? Tim supervised the project. He provided direction and helped both McCluskey and me a great deal. He didn't directly push any commits to the repository, though, so I thought it was appropriate to list him an author on the article but not as an author of the source code.

I can change this if required, but I thought this was a good way of representing authorship!

This is fine in this case. We might suggest that Tim also be added as an author of the zenodo repo based on his role in the project (project contributions don't have to be commits to source code), and then the authors would be consistent, but it's up to you.

@jgostick
Copy link

Hi @RBrearton , thanks to the editorialbot I just noticed that only 1 of your references has a doi. Can update the paper to include these for each reference?

@jgostick
Copy link

Thanks @danielskatz, I guess we just have to deal with the doi thing and i'll be ready for 'dry run' acceptance time!

@RBrearton
Copy link

Hi there, I've just updated the DOI's (the Rigaku paper doesn't have a DOI). I've also added Tim as an author on the Zenodo repository (@danielskatz is right about authorship; the place for tracking commits to source code is github, not Zenodo).

@jgostick
Copy link

@editorialbot recommend-accept

@editorialbot
Copy link
Collaborator Author

Attempting dry run of processing paper acceptance...

@editorialbot
Copy link
Collaborator Author

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

OK DOIs

- 10.1038/s41586-020-2649-2 is OK
- 10.1107/S1600576714027575 is OK
- 10.1107/S1600577516009875 is OK
- 10.5281/zenodo.7105217 is OK
- 10.1016/j.nima.2014.07.029 is OK
- 10.1107/S0021889807045086 is OK
- 10.1107/S1600576718017296 is OK
- 10.1107/S0021889806005073 is OK
- 10.1107/S1600576718011974 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@editorialbot
Copy link
Collaborator Author

👋 @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof 👉📄 Download article

If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3553, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

@editorialbot editorialbot added the recommend-accept Papers recommended for acceptance in JOSS. label Sep 26, 2022
@arfon
Copy link
Member

arfon commented Sep 29, 2022

@editorialbot accept

@editorialbot
Copy link
Collaborator Author

Doing it live! Attempting automated processing of paper acceptance...

@editorialbot
Copy link
Collaborator Author

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@editorialbot
Copy link
Collaborator Author

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.04397 joss-papers#3563
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.04397
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? Notify your editorial technical team...

@editorialbot editorialbot added accepted published Papers published in JOSS labels Sep 29, 2022
@arfon
Copy link
Member

arfon commented Sep 29, 2022

@andyfaff, @daguiam – many thanks for your reviews here and to @jgostick for editing this submission! JOSS relies upon the volunteer effort of people like you and we simply wouldn't be able to do this without you ✨

@RBrearton – your paper is now accepted and published in JOSS ⚡🚀💥

@arfon arfon closed this as completed Sep 29, 2022
@editorialbot
Copy link
Collaborator Author

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.04397/status.svg)](https://doi.org/10.21105/joss.04397)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.04397">
  <img src="https://joss.theoj.org/papers/10.21105/joss.04397/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.04397/status.svg
   :target: https://doi.org/10.21105/joss.04397

This is how it will look in your documentation:

DOI

We need your help!

The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Jupyter Notebook published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review TeX Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials
Projects
None yet
Development

No branches or pull requests

7 participants