Skip to content

[REVIEW]: Dark-field X-ray microscopy visualization #5177

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

Closed
editorialbot opened this issue Feb 21, 2023 · 65 comments
Closed

[REVIEW]: Dark-field X-ray microscopy visualization #5177

editorialbot opened this issue Feb 21, 2023 · 65 comments
Assignees
Labels
accepted Jupyter Notebook Procfile published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Feb 21, 2023

Submitting author: @trygvrad (Trygve Magnus Ræder)
Repository: https://github.com/trygvrad/DF-XRM_viz
Branch with paper.md (empty if default branch):
Version: v2.1.0
Editor: @jgostick
Reviewers: @taw10, @marcocamma
Archive: 10.5281/zenodo.7792903

Status

status

Status badge code:

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

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

@taw10 & @marcocamma, 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 @taw10

📝 Checklist for @marcocamma

@editorialbot editorialbot added Jupyter Notebook Procfile Python review Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials labels Feb 21, 2023
@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

Checking the BibTeX entries failed with the following error:

Lexical or syntactical errors: 

@online{plotly 

@editorialbot
Copy link
Collaborator Author

Software report:

github.com/AlDanial/cloc v 1.88  T=0.80 s (17.5 files/s, 478275.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                           7            256            783           1856
SVG                              1              0              0            551
Jupyter Notebook                 1              0         378105            174
Markdown                         2             53              0            117
TeX                              1             14              0             98
YAML                             1              1              4             18
Bourne Shell                     1              1              0             11
-------------------------------------------------------------------------------
SUM:                            14            325         378892           2825
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

@editorialbot
Copy link
Collaborator Author

Wordcount for paper.md is 1319

@editorialbot
Copy link
Collaborator Author

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

@taw10
Copy link

taw10 commented Feb 21, 2023

Review checklist for @taw10

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/trygvrad/DF-XRM_viz?
  • 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 (@trygvrad) 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
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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, who the target audience is, and its relation to other work?
  • 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?

@taw10
Copy link

taw10 commented Mar 2, 2023

Minor comments on the paper text:

  • Line 24 "but is instead at an irrational angle of rotation": minor objection to "irrational". I think it's meant to mean "not a low-index crystallographic direction", which I agree with. However there's certainly no requirement for the direction to be irrational (in a mathematical sense). Suggestion: replace with "..but instead can be at any orientation to the crystal", or similar.
  • Figure 2: Should be made larger, because the table is too small to read when the main text is at a comfortable size.
  • Line 79: Typo "diffracion" (should be "diffraction")
  • Line 93: Pedantic nitpick: The facility where DF-XRM was used, as described in the referenced paper (Holstad et al 2022), is the Linac Coherent Light Source (LCLS).
  • The syntax error noticed by the editorialbot should be fixed.
  • DOIs should be added for some of the references.

Installation and functional checks to follow soon!

@jgostick
Copy link

jgostick commented Mar 7, 2023

Hi @taw10 , thanks for you thorough review. @trygvrad, are you able to start address the @taw10's comments while we wait for the other reviewer to complete their review?

@marcocamma
Copy link

marcocamma commented Mar 7, 2023

Review checklist for @marcocamma

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/trygvrad/DF-XRM_viz?
  • 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 (@trygvrad) 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
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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, who the target audience is, and its relation to other work?
  • 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?

@taw10
Copy link

taw10 commented Mar 7, 2023

Comments after the functional test:

  • There is one step missing from the installation instructions for stand-alone installation, though it might be considered trivial: pip install -r requirements.txt. With this, the installation proceeds smoothly.
  • Perhaps include instructions on how to run the Jupyter example, though this might also be seen as trivial ("load it into JupyterLab" or equivalent).
  • Small fix needed in the Jupyter example (which worked, otherwise). Under "Plot absorbance", line 4:
    xtl.Scatter.setup_scatter(type='xray', energy_kev=energy_kev)
    Change type to scattering_type.
  • There are examples (which worked!), but no automated tests. Since most of the code is concerned with visualisation, it might be quite difficult to test thoroughly. But perhaps an automated end-to-end test would be appropriate. e.g. generating a PDF from one of the example files, which exercises a lot of the code.
  • Contribution guidelines are a little hard to find. There is a sentence at the bottom of the page when running the application ("please report bugs to [address] or contribute directly at github."), but nothing in the README.md, and no CONTRIBUTING.md.

@trygvrad
Copy link

trygvrad commented Mar 7, 2023

@taw10 Thank you for the comments
@jgostick I will start making adjustments tomorrow :)

@marcocamma
Copy link

marcocamma commented Mar 7, 2023

Functionality

  • maybe I missed it but I cannot find clear installation instructions. They are quite easy to guess for those who have similar installs in the past but they could be described in a separate INSTALL file or as part of the README.md. It should the different steps (clone repository, installing with pip install -r requirements. It could be mentioned the creation of the virtual environment.
  • when I try to install on a new venv (created with python3 -m venv my_env), several errors are shown; I do not know (yet?) if these matter

@trygvrad
Copy link

trygvrad commented Mar 8, 2023

Thank you for all the comments so far
@taw10 @marcocamma I added some install instructions that also show launching streamlit and/or jupyter

@marcocamma I was able to reproduce the error you found when installing the requirements. It did not impact performance but I added 'wheel' to the requirements, which will suppress the error.

I agree that an end-to-end test would be suitable, and I will add that later this week

@trygvrad
Copy link

trygvrad commented Mar 8, 2023

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@trygvrad
Copy link

trygvrad commented Mar 8, 2023

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@marcocamma
Copy link

@trygvrad , I really like the idea of the TOML file for the lenses ...
On my computer the white text on the light yellow background makes very difficult to see the "Upload a .lens file or select below" text (see screenshot below).
This happens only with the dark theme (unfortunately was the default so I had to spend time before realizing there was a text message!)

2023 03 08_22h03_screenshot

@marcocamma
Copy link

@trygvrad , nice work !
I understand the interest of a GUI, yours is also nice and polished but it would it make sense to highlight that the package can be used as "library" as shown in the jupyter notebook example !
I agree with @taw10 that some automatic testing would be good; if I understood well the jupyter example, there is not need to visualize to get values (reflection to use, lens distances, etc.) to use in automatic testing.
Precalculating few usecases and hardcoding the results in a test might be a way to make sure that future changes do not break the code. Generating a pdf can indeed be nice to exercise the "GUI" aspects.

I will keep trying things and exercise the different aspects a bit before closing the review

@trygvrad
Copy link

Thank you @taw10 and @marcocamma for all the comments,

I did a number of changes:

  • I added a CONTRIMBUTE.md with information regarding contribution

  • I corrected the errors in the manuscript

  • I added an automated test. An automated test is a very good idea in this case as any changes to the master branch is automatically applied to the live streamlit page, which may at any time be used for an ongoing experiment. I therefore made quite an agressive test to act as a speed bump when making changes. The test compares both numerical values, the images, and makes a pdf that can be downloaded on the 'actions' page. A note on the test is included in CONTRIMBUTE.md.

  • Thank you for alerting me to the fact that dark mode was not really functional. Apparently streamlit will default to dark mode if your browser is set to dark mode. I had changed the background color so that it was clear what was a figure and what was background when using the page in light mode, which had unintended consequences for dark mode. I changed my approach to this and the streamlit app now works in both light and dark mode.

@marcocamma
Copy link

I have played a bit more with the program and I don't see much else to improve. Thanks for taking account my suggestion.

@jgostick
Copy link

This submission looks to be moving along well. I see that @taw10 has a few checkboxes left but I think these have been addressed. I think we can declare this submission as ready. There are a few pre-acceptance checks that need to be done first though. I will generate a checklist in the box below.

@jgostick
Copy link

@editorialbot generate post-review checklist

@editorialbot editorialbot added the recommend-accept Papers recommended for acceptance in JOSS. label Apr 2, 2023
@jgostick
Copy link

jgostick commented Apr 2, 2023

@trygvrad, this submission is now in acceptable form. An editor in chief will come and make it official.

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot set 10.5281/zenodo.7792903 as archive

@editorialbot
Copy link
Collaborator Author

Done! Archive is now 10.5281/zenodo.7792903

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Apr 5, 2023

@jgostick thanks for your help editing here! Some minor feedback:

  • Note in the future please leave out the https://doi.org/ when setting the archive DOI, so in this case just 10.5281/zenodo.7792903. Thanks
  • The title on the archive does not match.
  • The version tag is here set as 2.1.0 but should match the tag on the repository and archive which includes the v, i.e. v2.1.0

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot set v2.1.0 as version

@editorialbot
Copy link
Collaborator Author

Done! version is now v2.1.0

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Apr 5, 2023

@trygvrad I am the AEiC on this track and here to help process the final steps.

  • Can you please edit the ZENODO archive title to match your paper title? I've unticked the corresponding box up there ☝️ , so you may tick it again when done. Thanks.
  • Fix the spelling for differnt in the paper
  • Words like localized and optimize use the American English spelling. However you also use the British English spelling programme. Consider updating to use one form throughout.

@trygvrad
Copy link

trygvrad commented Apr 7, 2023

@Kevin-Mattheus-Moerman I updated the title at the zenodo archive and fixed the spelling.
My apologies for the errors.
Note that I did not create a new release, so the spelling errors are still present in the version of the paper included in the zenodo archive. Let me know if you want me to create a new release (this will create a new DOI).

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@Kevin-Mattheus-Moerman
Copy link
Member

@trygvrad thanks for making those changes. No new release is needed. We are good to proceed now.

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot accept

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository.

If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file.

You can copy the contents for your CITATION.cff file here:

CITATION.cff

cff-version: "1.2.0"
authors:
- family-names: Ræder
  given-names: Trygve Magnus
  orcid: "https://orcid.org/0000-0003-3524-362X"
doi: 10.5281/zenodo.7792903
message: If you use this software, please cite our article in the
  Journal of Open Source Software.
preferred-citation:
  authors:
  - family-names: Ræder
    given-names: Trygve Magnus
    orcid: "https://orcid.org/0000-0003-3524-362X"
  date-published: 2023-04-15
  doi: 10.21105/joss.05177
  issn: 2475-9066
  issue: 84
  journal: Journal of Open Source Software
  publisher:
    name: Open Journals
  start: 5177
  title: Dark-field X-ray microscopy visualization
  type: article
  url: "https://joss.theoj.org/papers/10.21105/joss.05177"
  volume: 8
title: Dark-field X-ray microscopy visualization

If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation.

Find more information on .cff files here and here.

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

🐘🐘🐘 👉 Toot 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.05177 joss-papers#4126
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.05177
  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 Apr 15, 2023
@Kevin-Mattheus-Moerman
Copy link
Member

@trygvrad congratulations on this publication.

Thank you for editing @jgostick !

And a special thanks to the reviewers @taw10 and @marcocamma

@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.05177/status.svg)](https://doi.org/10.21105/joss.05177)

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

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

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:

@trygvrad
Copy link

Thank you everyone,
I must say this has been the most pleasant review process I have ever encountered.

@jgostick
Copy link

Thanks to the reviewers for their time and efforts. Sorry for my slowness at times...this role gets back-burned from time to time. Glad it went smoothly for you.

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

No branches or pull requests

6 participants