search

openjournals / joss-reviews

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

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

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@RBrearton<!--end-author-handle-- (Richard Brearton) Repository: https://github.com/RBrearton/islatu Branch with paper.md (empty if default branch): islatu_paper Version: 1.0.7 Editor: !--editor-->@jgostick<!--end-editor-- 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 commented 2 years 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 2 years ago 
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 commented 2 years ago 
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 commented 2 years ago

Wordcount for paper.md is 1011

editorialbot commented 2 years ago

Failed to discover a valid open source license

editorialbot commented 2 years ago

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

andyfaff commented 2 years ago

Review checklist for @andyfaff

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

andyfaff commented 2 years ago

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 commented 2 years ago

@editorialbot add @daguiam as reviewer

editorialbot commented 2 years ago

@daguiam added to the reviewers list!

jgostick commented 2 years ago

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 commented 2 years ago

Review checklist for @daguiam

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jgostick commented 2 years ago

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 commented 2 years ago

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

RBrearton commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

daguiam commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

jgostick commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

jgostick commented 2 years ago

Comments on your paper:

andyfaff commented 2 years ago

I don't think refnx has to be cited, but it would be good to mention other reduction codes apart from islatu, even if it's e.g. something like reductus.

RBrearton commented 2 years ago

Hi @jgostick, just had some time to go through this.

It seems like the summary section ends abruptly. It feels like it needs a closing sentence like. ...

Yep, true. Added a closing sentence which also hopefully clarifies where islatu lives in the reflectivity software stack.

"simplify the handling of errors" should probably be "handling of uncertainties" or "experimental errors"?

Yep, changed it to "uncertainties" and it's much clearer.

Is it possible to put a screen shot or some sort of image in the paper?

Yeah... good point. I was lazy with this because the documentation is (hopefully!) easy to find, and technical details can be found there.

Your "statement of need" section points out why someone might need software to do this analysis but should also or alternatively focus on why islatu needed to be written.

Added a final paragraph to the statement of need section which hopefully satisfies this requirement.

Are there really no other open source codes? There is no mention of refnx in the paper, which seems odd. Perhaps this is why @andyfaff has left the Statement of need checkbox open?

There's really not much. refnx is, primarily, a fitting program that one would use to analyse a reflectivity curve. The problem is that this isn't what your instrument gives you. Islatu was born out of a need to map large numbers of pixels with meaningless intensities to physically meaningful reflectivity curves, which a user can then plug into a tool like refnx.

Integrating with something like reductus would be awkward, because the philosophy of the program is different. Reductus is a really nice graphical way to reduce (currently neutron) data. Islatu is supposed to be small and specialised ā€“ only as large as is required to convert meaningless pixels into a meaningful curve. At that point, you can use your favourite reflectivity fitting program to extract physically meaningful information from your measured data.

I hope this makes the wording in the paper make more sense! I also hope that this round of changes to the paper make this clearer.

The paper mentions a few things about performance with regard to error propagation ("such that the number of mathematical operations required to propagate the errors is minimised"). Can you elaborate on this at all?

Well, as with all things, it's possible to write code that does the job but is unbelievably slow. I'm just trying to emphasise here that there was an eye on performance when writing the code. For example, initially we used https://pypi.org/project/uncertainties/ for error propagation but it was sickeningly slow (like 2 orders of magnitude slower than it is now).

Other than that, the phrase about minimising mathematical operations is correct. Wherever possible, tricks with poisson stats were played to decrease the number of times that errors needed to be calculated (e.g. if it's possible to skip calculating errors at some intermediate point in the calculation and compensate for it later, then this will be done). I don't mean that some incredible novel algorithm has been implemented, the point is more to emphasise that thought has been put into making this reasonably quick.

Overall the paper is quite short. I know that JOSS explicitly states that it's about the software not the paper, but this is a chance for you to create a permanent record describing the software. You maybe want to take this opportunity to pad the paper a little bit? Again this is optional.

If it's possible, I'd prefer to pass on this. Because this is a permanent record, I don't want to pad the paper with specific details that might change in the future. I personally like the idea of something short, sweet and fairly permissive with regards to future changes. I'd like to think that, right now, a reader would get the impression that this is a relatively simple library that maps raw data to R(Q) reasonably quickly; this is exactly what I'd like to get across!

Do let me know what you think of the changes, and what else needs to be done.

jgostick commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

jgostick commented 2 years ago

The paper looks great, the choice of before-after data is very convincing!

Regarding performance, @daguiam had not check this box, but between your comments and @andyfaff's endorsement, I think I am satisfied

Regarding citing relevant softare, I see that you've added references to several existing packages, so I considered @andyfaff's unchecked box on this issue to be satisfied as well.

Overall, I think we're ready to go! There are a few additional steps though, such as declare a final version number, and minting a doi for the code itself. I will put all this in the form of a checklist below.

jgostick commented 2 years ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

jgostick commented 2 years ago

Hi @RBrearton, how are things progressing?

RBrearton commented 2 years ago

Hi @jgostick, sorry for the slow response, I've been delayed by the fact that Zenodo is broken atm (it isn't possible to upload files).

At first I thought this was just me being an idiot, but they did post on twitter on the 1st of September that file uploads aren't working, so I suppose it isn't just me.

When Zenodo is back up, I'm ready to make a release number 1.0.7 which will sync with pypi etc. including all changes made in response to this review.

jgostick commented 2 years ago

@RBrearton have you tried again lately? The site looks ok now. There are uploads with recent dates.

RBrearton commented 2 years ago

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 commented 2 years ago

@editorialbot set 1.0.7 as version

editorialbot commented 2 years ago

Done! version is now 1.0.7

jgostick commented 2 years ago

@editorialbot set 10.5281/zenodo.7105217 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.7105217

jgostick commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

jgostick commented 2 years ago

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

RBrearton commented 2 years ago

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 commented 2 years ago

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

jgostick commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
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 commented 2 years ago

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 commented 2 years ago

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?