openjournals / joss-reviews

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

[REVIEW]: APAV: Python for Atom Probe Tomography #4862

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@jdasm<!--end-author-handle-- (Jesse Smith) Repository: https://gitlab.com/jesseds/apav Branch with paper.md (empty if default branch): JOSS Version: 1.4.0 Editor: !--editor-->@rkurchin<!--end-editor-- Reviewers: @ziatdinovmax, @mkuehbach Archive: 10.5281/zenodo.7738382

Status

status

Status badge code:

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

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

@mkuehbach & @ziatdinovmax, 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 @rkurchin 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 @ziatdinovmax

📝 Checklist for @mkuehbach

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.52 s (150.9 files/s, 23570.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          40           1764           2088           5048
reStructuredText                21            495            219           1458
SVG                              6              3              3            376
TeX                              1             10              0            121
Markdown                         2             49              1            119
Cython                           2             43             43            118
YAML                             2             16              2            115
DOS Batch                        2             12              1             51
make                             1              4              7              9
TOML                             1              1              0              5
-------------------------------------------------------------------------------
SUM:                            78           2397           2364           7420
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1761

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

OK DOIs

- 10.1038/s43586-021-00047-w is OK
- 10.1038/s41524-020-00486-1 is OK
- 10.1016/j.ultramic.2010.11.021 is OK
- 10.1017/S1431927621012794 is OK
- 10.5281/zenodo.5570790 is OK
- 10.1080/09500830903472997 is OK

MISSING DOIs

- None

INVALID DOIs

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

rkurchin commented 1 year ago

@aliusdragonfly, @ziatdinovmax, let me know if you have any questions about how to get started on your reviews!

aliusdragonfly commented 1 year ago

Haha. No questions on how, just trying to line up a slot for when...

Should be able to get to it Friday, at least to make a start.


From: Rachel Kurchin @.> Sent: Tuesday, October 25, 2022 6:07 PM To: openjournals/joss-reviews @.> Cc: Akey, Austin @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: APAV: Python for Atom Probe Tomography (Issue #4862)

@aliusdragonflyhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_aliusdragonfly&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=i1XD4vrFod9g3xujqbMDqsMaafxF0kWNw9QOfOvB9xE&m=sZPkDoMuTkm0JHfm49t9SmbhTjzwoc9JqgPQT2F6rczrCgq_s4K_NU9fBdM3VyRx&s=fJ4xQ6GReABHzHHdRfxP42FF6kmWw_fFTtBAlkpTT_M&e=, @ziatdinovmaxhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ziatdinovmax&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=i1XD4vrFod9g3xujqbMDqsMaafxF0kWNw9QOfOvB9xE&m=sZPkDoMuTkm0JHfm49t9SmbhTjzwoc9JqgPQT2F6rczrCgq_s4K_NU9fBdM3VyRx&s=fryYVZrqutNF-cwdIugH6-O7f4RO74n5WlA0URvMtEc&e=, let me know if you have any questions about how to get started on your reviews!

— Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openjournals_joss-2Dreviews_issues_4862-23issuecomment-2D1291193736&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=i1XD4vrFod9g3xujqbMDqsMaafxF0kWNw9QOfOvB9xE&m=sZPkDoMuTkm0JHfm49t9SmbhTjzwoc9JqgPQT2F6rczrCgq_s4K_NU9fBdM3VyRx&s=GCnOAQ_W8gMWDYY5JXOXSJr3w0tAVaVaowesn2i8mDc&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_A3RX75H75SVRLTH4PWKNU4TWFBKY5ANCNFSM6AAAAAARHK6JGQ&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=i1XD4vrFod9g3xujqbMDqsMaafxF0kWNw9QOfOvB9xE&m=sZPkDoMuTkm0JHfm49t9SmbhTjzwoc9JqgPQT2F6rczrCgq_s4K_NU9fBdM3VyRx&s=pEkdmc2wHfFirNX4PxH43YVuS3bFX0J6bK0gFvN6dy0&e=. You are receiving this because you were mentioned.Message ID: @.***>

rkurchin commented 1 year ago

Hi again reviewers (@aliusdragonfly @ziatdinovmax), reminder to get reviews started! As you work on your checklist, feel free to file issues in the project repository; if you do so, please link back to this review issue for easy tracking.

ziatdinovmax commented 1 year ago

@rkurchin I apologize for the delay. I will try to review it before the end of the week.

rkurchin commented 1 year ago

Thanks for getting started, @aliusdragonfly – reminder that as you work through the checklist, you can feel free to file issues in the source repository, please link back to this issue for easy tracking (though I'm not actually sure if the tracking will work as well between GitHub and GitLab 🤷‍♀️).

@ziatdinovmax, checking in with you as well...

rkurchin commented 1 year ago

🔔 Checking in again with reviewers, @ziatdinovmax and @aliusdragonfly...

ziatdinovmax commented 1 year ago

Review checklist for @ziatdinovmax

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ziatdinovmax commented 1 year ago

@rkurchin Going through the examples now. Quick question to @jdasm: could you please point me to the location of the files needed to reproduce the examples in Multiple Detection Events, Spectrum Analysis, and Spatial Analysis? ('GBCO.epos', 'GBCO.pos', 'R5038_00556-v04.epos', 'rng_5pj.rrng') Thanks!

jdasm commented 1 year ago

Hi @ziatdinovmax , thank you for taking the time to review the paper. I have provided download links for these 4 files below, they are not present within the repository due to their size (600MB - 3GB, except for the rrng file). The "GBCO" file is a bit of a generic name for the material system from which the data is generated, but in most cases it should refer to the particular dataset listed below.

R5038_00556-v04.epos: http://gofile.me/72LyR/8Db4niyfS rng_5pj.rrng: http://gofile.me/72LyR/5tHuGKKvg GBCO (R5038_00333-v02.epos): http://gofile.me/72LyR/UCHyUfvLp GBCO (R5038_00333-v02.pos): http://gofile.me/72LyR/0fLmUGmu0

Thanks!

rkurchin commented 1 year ago

Hey @aliusdragonfly, just checking in again on your review progress!

rkurchin commented 1 year ago

Pinging reviewers @aliusdragonfly and @ziatdinovmax again to check in!

rkurchin commented 1 year ago

Hi @aliusdragonfly and @ziatdinovmax, please do let me know if you need any help continuing your reviews!

rkurchin commented 1 year ago

Happy New Year, everyone! Just checking in on this...

@aliusdragonfly, friendly reminder to continue your review and let me know if you have any questions about the process.

@ziatdinovmax, I see your checklist is complete, thanks! Just to confirm: are you happy to recommend publication at this point?

ziatdinovmax commented 1 year ago

@rkurchin - yes, it all looks good, and my apologies for the slow review

rkurchin commented 1 year ago

Just sent @aliusdragonfly an email in case his GitHub notifications got borked...

rkurchin commented 1 year ago

Emailed again...

jdasm commented 1 year ago

Tagging @mkuehbach here

rkurchin commented 1 year ago

@editorialbot remove @aliusdragonfly as reviewer

editorialbot commented 1 year ago

@aliusdragonfly removed from the reviewers list!

rkurchin commented 1 year ago

@editorialbot add @mkuehbach as reviewer

editorialbot commented 1 year ago

@mkuehbach added to the reviewers list!

rkurchin commented 1 year ago

@mkuehbach, thanks so much for your willingness to step in as another reviewer here! I've had to manually edit the first comment. You should be able to generate your own checklist (I may have to edit the first comment to create the link to it) and start your review whenever you're ready! Please feel free to let me know if you have any questions about the process. Note that you should feel free to file issues in the host repository; please link to this review issue if you do so to facilitate tracking.

mkuehbach commented 1 year ago

Dear @rkurchin, sure I am happy to review this work. Could you point me to the current checklist which I should use and work through to perform the revision. This is my first time to review for JOSS. Thank you very much.

rkurchin commented 1 year ago

If you read over the top comment on the review, you'll see a comment you can enter to generate your review checklist (along with some other general guidance).

mkuehbach commented 1 year ago

Review checklist for @mkuehbach

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

mkuehbach commented 1 year ago

Comment on the paper draft manuscript: https://gitlab.com/jesseds/apav/-/blob/JOSS/joss/paper.md, 71b90e9e8b63fa2e88df5ec4a0ea0246c63cd262

Title:

Should be more specific

Summary:

Solid-state microstructures (omit solid-state) because which other microstructures exists to make this distinction? "Composition" rather than chemical because the strength is mainly in the analysis of composition rather than actual bonds between atoms which would be the field of (physical) chemistry. "complex data analysis/visualization", not to forget technical constraints put in place by technology partners. "multidimensional" too much advertisement type phrase APT reconstructions are 3D time is sampled if at all quasi in-situ. "Efficient" in terms of what?

Statement of need:

"Conversely, the landscape of open-source tools for APT analysis is very sparse with limited options [@3depict; @paraprobe]" This sentence cannot remain like this. Options in tools are always limited. Neither are the options in the here mentioned tools seemingly narrow, as the tone of the sentence indicates nor is the list of two tools mentioned here an up-to-date statement of the in fact quite large number of tools available. The author should rather cite publicly available lists of software tools for atom probe and state that among the set of tools there are gaps which APAV covers, namely the field of open Python tools for atom probe in general and for mass spectrum and multihit analyses specifically.

"minimize the knowledge barrier" in fact properly written Python code is not necessarily cleaner or simpler to understand by non-programmers than is code written in other programming language. The author is right though that the availability of libraries, the open-source nature of the ecosystem is a substantial strength which makes Python worth to consider an ecosystem for writing software for atom probe in. The reviewer is convinced that working towards a set of specifically customized libraries "wrappers" to and using Python libraries which implement the specifically customized algorithms used in atom probe, should be implemented in Python. Only where alternative solutions are absent or specific numerical tasks with specific performance required compiled code should be used. In fact the idea of a library for atom probe was already put forth much earlier by e.g. D. Haley and A. London called libatomprobe (see on sourceforge).

Input/Output:

File formats, could also point to B. Gault 2012 and D. Larson 2013 books with DOIs. In the list of pre-reconstruction (*.str would one more worth to be mentioned) What do the abbreviations mean.

DME:

"(due to weak electric-field screening)" this statement demands literature backup "higher rate" or "higher fraction" of multiples? "can result in data loss" grammar "molecuar" typo numbers smaller than 12 to be written out correlation histograms (statement needed making reference to key work in the literature D. Saxey et al.)

Mass spec:

consistency "nonmetals" vs "non-metals" throughout the manuscript

Delocalization:

"direction" of what this is unclear, direction of the research? Avoid the in-line formatting with 1) and 2), rather write one sentence, like firstly, secondly. What is 3sigma (three times standard deviation)? It should also be mentioned that this smoothening is not directly related to the actual imprecision/inaccuracy of how atoms were reconstructed.

Acknowledgements:

Is there funding to be mentioned for this work?

mkuehbach commented 1 year ago

I was able to access the example data and will go about testing the code next to give reviewer feedback on the remaining points on the checklist.

mkuehbach commented 1 year ago

Okay, now on the remaining feedback to complete this revision round:

Testing the tools:

mkuehbach commented 1 year ago

Based on my experiences (see comments and suggestions above), I would like to thank the author for building what is overall a well-documented and clean Python package. I am convinced that it can find its place as a component in the open-source atom probe data analysis landscape. I would be happy to hear back from the author when the respective comments have been edited and the manuscript and repository been updated accordingly. If this has happened, I would be happy to tick the remaining checklist points; and provided these edits enter the paper, I am happy to suggest then that it is ready for making the final decision @rkurchin as an editor on the publication.

jdasm commented 1 year ago

Thanks @mkuehbach for taking the time to provide valuable feedback to both the paper and repository. Our input is given below. All changes are made to the joss branch in the repository (https://gitlab.com/jesseds/apav/-/tree/JOSS)

Paper review

Title:

Should be more specific

We have changed the title to: “APAV: An Open-Source Python Package for Spectral Analysis in Atom Probe Tomography”

Summary:

Solid-state microstructures (omit solid-state) because which other microstructures exists to make >this distinction?

We have changed “Solid-state microstructures” to just “microstructures”, removing the unnecessary qualifier.

"Composition" rather than chemical because the strength is mainly in the analysis of composition rather than actual bonds between atoms which would be the field of (physical) chemistry.

Agreed, we have changed “chemical” to “compositional”.

"complex data analysis/visualization", not to forget technical constraints put in place by technology partners.

We have added a statement to the end of the sentence to cover this facet:

“However, the informative potential of APT is hindered by nebulous, material-dependent evaporation physics, naturally complex data analysis/visualization, and the standards/specifications set by vendors.”

"multidimensional" too much advertisement type phrase APT reconstructions are 3D time is sampled if at all quasi in-situ.

We have removed the word “multidimensional”.

"Efficient" in terms of what?

We agree “Efficient” is unnecessarily vague and decided to just remove the word. The sentence now reads “Transparent and accessible data analysis tools ...” instead of “Efficient, accessible, and transparent data analysis tools...”

Statement of need:

"Conversely, the landscape of open-source tools for APT analysis is very sparse with limited options [@3depict; @paraprobe]" This sentence cannot remain like this. Options in tools are always limited. Neither are the options in the here mentioned tools seemingly narrow, as the tone of the sentence indicates nor is the list of two tools mentioned here an up-to-date statement of the in fact quite large number of tools available. The author should rather cite publicly available lists of software tools for atom probe and state that among the set of tools there are gaps which APAV covers, namely the field of open Python tools for atom probe in general and for mass spectrum and multihit analyses specifically.

We have re-written the sentence to be more comprehensive and evenhanded in our assessment of APT tools, which now reads:

“Generalized lists of open-source contributions may be found online, in addition to a collection of software-oriented journal articles (Felfer e tal., 2013; Kubach et al., 2021; Monajem & Felfer, 2022). APAV aims to improve the general coverage of APT tools in the Python space while specifically targeting the analysis of mass spectra.”

(there is a footnote in this sentence pointing to one such list, but cannot appropriately show this in markdown)

We did not intend to classify any project as narrow, particularly the two referenced (apav also has its own topical specializations). In fact, our intention was to highlight some of the more matured projects exhibiting “production” qualities like testing, documentation sites, tagged releases, and such. These qualities tend to inspire confidence in a project’s functions but are perhaps too often forgone in research software. This filter for which we distilled the software ecosystem in this sentence was not conveyed and is perhaps unneeded.

At the reviewer’s suggestion we came across this page (https://github.com/FAIRmat-Experimental/software-tools/blob/main/software-list-b5-atom-probe-microscopy.md) listing many open-source projects of varying maturity and scope. We were unaware of many of these since they are not so organically found (within the context of SEO or having an indexed project/documentation page); although, they are more easily discovered through deliberate queries on each site (GitHub, Source Forge, GitLab, etc.). We have referenced this page as a footnote for inquiring readers.

"minimize the knowledge barrier" in fact properly written Python code is not necessarily cleaner or simpler to understand by non-programmers than is code written in other programming language. The author is right though that the availability of libraries, the open-source nature of the ecosystem is a substantial strength which makes Python worth to consider an ecosystem for writing software for atom probe in. The reviewer is convinced that working towards a set of specifically customized libraries "wrappers" to and using Python libraries which implement the specifically customized algorithms used in atom probe, should be implemented in Python. Only where alternative solutions are absent or specific numerical tasks with specific performance required compiled code should be used. In fact the idea of a library for atom probe was already put forth much earlier by e.g. D. Haley and A. London called libatomprobe (see on sourceforge).

We intended to allude that interpreted and non-statically typed languages are typically easier to learn (Matlab, R, Python, etc.), resulting in a higher likelihood that non-computer scientist researchers would favor/be skilled in these languages (at least by the author’s own observations of his colleague's language choices). That is not to say that interpreted languages are better in any comprehensive sense (they are not). We have re-written this sentence to more intently and less implicitly describe our motivation for targeting Python, now reading:

“Python is a popular language in both general and specialized computing domains, partly owing to its comprehensive centralized package management (PyPi and Conda), openness, and reputed ease of use. Such qualities have helped to ingrain Python into the greater academic community, forming our belief that the development of APT tools for Python should be pursued.”

Input/Output:

File formats, could also point to B. Gault 2012 and D. Larson 2013 books with DOIs.

We have added these 2 references.

In the list of pre-reconstruction (*.str would one more worth to be mentioned) What do the abbreviations mean.

We have added expansions of the (post-reconstruction) abbreviated file types:

“For best interoperability, APAV does not introduce new file formats, instead supporting the major file formats: .pos (position), .epos (extended position), .ato (atom), .apt (atom probe tomography), .rng (range), and .rrng (range).”

And added *.str to the following sentence.

DME:

"(due to weak electric-field screening)" this statement demands literature backup

We added the following reference which partly studies the internal electrostatic distributions arising from fields applied to a metal and MgO:

Tamura, H., Tsukada, M., McKenna, K. P., Shluger, A. L., Ohkubo, T., & Hono, K. (2012). Laser-assisted field evaporation from insulators triggered by photoinduced hole accumulation. In Physical Review B (Vol. 86, Issue 19). American Physical Society (APS). https://doi.org/10.1103/physrevb.86.195430

"higher rate" or "higher fraction" of multiples?

We have just generalized this to “increased”.

"can result in data loss" grammar

Changed “can result data loss” to “may result in data loss”.

"molecuar" typo

Fixed

numbers smaller than 12 to be written out

Fixed

correlation histograms (statement needed making reference to key work in the literature D. Saxey et al.)

This reference is present in the second/last paragraph of the DME section, but we moved it to a slightly better place within the sentence.

Mass spec:

consistency "nonmetals" vs "non-metals" throughout the manuscript

All instances have been set to “nonmetal”.

Delocalization:

"direction" of what this is unclear, direction of the research?

We replaced this with “the exposed surface crystallography”. The whole sentence reads:

“APT is often considered to exhibit atomic resolution, however, this truly depends on the specimen, acquisition parameters, and the exposed surface crystallography.”

Avoid the in-line formatting with 1) and 2), rather write one sentence, like firstly, secondly.

We changed the sentence:

“The compositional grid is created in a two-step process: 1) Delocalization of each ion into bins using a transfer function and 2) Smoothing the resulting grids (typically a gaussian kernel).”

To:

“The compositional grid is initialized by the delocalization of each ion into discrete bins using a chosen transfer function, which is then smoothed (typically using a gaussian kernel).”

What is 3sigma (three times standard deviation)? It should also be mentioned that thiscsmoothening is not directly related to the actual imprecision/inaccuracy of how atoms were reconstructed.

We have added a clarification following the first usage of the term changing:

“...which expresses the 3σ spatial distance that each...”

To

“...which expresses the 3σ (three standard deviations) spatial distance that each...”

We also added a new sentence to clarify the purpose of the smoothing step:

“This smoothing is applied to mitigate aliasing artifacts possibly introduced by the grid-based sampling process of the ion distribution (and is not related to the detection process itself).”

Acknowledgements:

Is there funding to be mentioned for this work?

There is no funding to attribute this work to.

Code review

Note: All code changes made in the course of the review are made to the joss branch, all diffs between the joss branch and latest release can be viewed here: https://gitlab.com/jesseds/apav/-/compare/master...JOSS?from_project_id=10414782&straight=false

The review related documention build is here: https://apav.readthedocs.io/en/joss/

I recommend to point here to the DOI and the literature instead of uploading the file because this may qualify as a copyright issue

Agreed, we have removed the file from the repository and added a citation to the relevant documentation page (https://apav.readthedocs.io/en/joss/formats.html). An inquiring user can then download the same file from the linked publisher’s page.

I was able to install the tools on Win11/WSL2. Nothing is mentioned though that qt can be a showstopper unless a description instructs the unexperienced on how to get the qt X11 forwarding done correctly.

Reading the hint in the CI workflow yaml revealed that mesa system libraries should be installed. This has to be mentioned in a more prominent place for end users! The authors should clarify the relevant thirdparty OS dependencies and add a clear description here in the docs under e.g. installation or maybe prerequisites.

We added a section for WSL2 and bare linux systems on the installation page.

At the outset, we did not anticipate interest in WSL, as opposed to native usage in Windows. Standard (non-virtual and non-headless) installations of the popular Linux distributions also typically have these dependencies satisfied after completed setup (of the Linux distribution), so we did not originally consider these details. To support users in these environments (like WSL), we have added the necessary details regarding these OS dependencies, including an example of setting up apav in a fresh WSL2. https://apav.readthedocs.io/en/joss/install.html#windows-subsystem-for-linux-and-bare-linux-systems

Using that fresh py38 venv showed that matplotlib was not installed along the way. Thus issues were faced while running the code but they were easily fixable by a pip install matplotlib so add it to the requirements.txt

Here the author should check again pedantically which libraries are needed.

Jupyter notebooks are mentioned in the docs, but not installed by default, this should be added.

We did not force these packages (matplotlib and Jupyter) as dependencies since we consider them personal choices by the user regarding their own predilections toward plotting libraries and development environments (neither of these packages are strictly required for apav to function). Matplotlib is simply a canonical plotting library we chose for plotting results in the documentation. Jupyter notebooks seem more of an optional component of the development environment rather than the project itself (despite our own recommendation in the documentation).

I think the best solution is a separate dependency group for bringing in these optional dependencies; I.e., pip install apav[opt]. This allows us to maintain a minimal dependency size for the core package while also allowing a single-line installation of apav to include these (and future) optional dependencies whenever needed.

We have added a new [opt] dependency group (to setup.cfg), and a mention of this in the installation page of the documentation (https://apav.readthedocs.io/en/joss/install.html#optional-dependencies).

I saw some import * lines, which should be avoided and be formulated as cleaner modules.

We have removed the majority of wildcard import statements. We describe the details and changes to the 2 instances where we regularly used such imports below.

from apav.qt import *

Qt classes and namespaces are partially qualified by a prepended “Q” (I.e., QGraphicsItemGroup), making name clashes easily avoidable while maintaining clarity over whether an object belongs to Qt; though of course, it is still better avoided. We have eliminated the wildcard importing of this module but left the wildcard imports within the module itself (I.e., from PyQt5.QtCore import *, from PyQt5.QtGui import *, etc. inside apav.qt).

from apav.utils.hinting import *

Here we often load a small collection of python types for type hinting purposes. We have replaced this with explicit imports at the top of each file.

There is a typo in the Ba code line in the spatial example which can easily be fixed.

Thanks, the erroneous parenthesis was removed.

jdasm commented 1 year ago

@editorialbot generate pdf

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:

rkurchin commented 1 year ago

@mkuehbach have you had a chance to look at the changes @jdasm outlined above?

rkurchin commented 1 year ago

Hi @mkuehbach, just checking in on this again!

mkuehbach commented 1 year ago

@rkurchin after conferences, illness, and many other points finally: I went through the authors changes and agree to all of these. I have four minor points which I would like to suggest. The authors should check these in their own interest:

This is a very useful and neat work Jesse and Marcus!

We would are looking forward to connecting it to the NOMAD Oasis containers.

mkuehbach commented 1 year ago

@rkurchin nothing more from my side, thanks for organizing it

rkurchin commented 1 year ago

Great, thanks @mkuehbach. Can you finish off your editorial checklist (the big one further up) in that case? In the meantime, I'll start finalizing this submission!

rkurchin commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

rkurchin commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1038/s43586-021-00047-w is OK
- 10.1017/S1431927622003397 is OK
- 10.1038/s41524-020-00486-1 is OK
- 10.1016/j.ultramic.2010.11.021 is OK
- 10.1017/S1431927621012794 is OK
- 10.5281/zenodo.5570790 is OK
- 10.1080/09500830903472997 is OK
- 10.1103/PhysRevB.86.195430 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.ultramic.2013.03.004 is INVALID because of 'https://doi.org/' prefix
rkurchin commented 1 year ago

@jdasm please fix that one DOI when you have a moment

rkurchin commented 1 year ago

@editorialbot generate pdf

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:

rkurchin commented 1 year ago

Some small editorial suggestions:

jdasm commented 1 year ago

Thanks again everyone for the input.

@mkuehbach

Title: "Spectral" these are not really spectral analyses "Spectrum"?

We changed the title to signify mass spectrum instead of spectra.

Make a downlaod link on e.g. Zenodo where all example data can be loaded for testing and trying out and mention this in the text.

We uploaded the documentation data to Zenodo and added a note before the first usage of any of these files in the documentation (https://apav.readthedocs.io/en/joss/roi.html#basic-properties).

I recommend to check the references because here are some errors with the author names and their abbreviations. Also check that you have DOIs for all of the refs. I recommend to download the software list from FAIRmat-Experimental and put it in this repository (you can leave a comment in the text like "(compiled by the FAIRmat project of the German NFDI)". Reason is we might experience some project repo reorganizations soonish, so it would be sad if these documents become invalid for your paper Jesse.

We have copied the file into the repository and updated the reference/footnote in the paper with a corresponding notice of the file's origin. Also, we took a pass over author name formatting in the bib file.

Sometimes APAV as a term is highlighted and sometimes not throughout your text. I think being consistent is better.

Thanks, I thought I did this before but must have mis-typed the search/replace string.

@rkurchin

We have made the editorial changes and fixed the improperly formatted DOI.

If we are set then I can merge the changes and make a new release tag.