8000 Joss paper dataconverter edit by sherjeelshabih · Pull Request #663 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Joss paper dataconverter edit #663

8000
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 7, 2025
Merged

Joss paper dataconverter edit #663

merged 8 commits into from
Jul 7, 2025

Conversation

sherjeelshabih
Copy link
Collaborator

The current count is at 991 in the tool I used.

@lukaspie I also reduced some parts in your section. Please have a look and feel free to edit or comment.

@sherjeelshabih sherjeelshabih changed the base branch from master to joss_paper July 1, 2025 13:22
@sherjeelshabih sherjeelshabih force-pushed the joss_paper_dataconverter_edit branch from a8a3184 to d390409 Compare July 1, 2025 13:27
lukaspie
lukaspie reviewed Jul 1, 2025
View reviewed changes
lukaspie
lukaspie reviewed Jul 1, 2025
View reviewed changes
lukaspie
lukaspie reviewed Jul 1, 2025
View reviewed changes
Copy link
Collaborator
@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only reviewed my part in-depth, but the rest looks fine.

As a side note, can you @sherjeelshabih collect everything that we are cutting it somewhere? I can then add it to the docs later on.

lukaspie
lukaspie reviewed Jul 1, 2025
View reviewed changes
@sherjeelshabih
Copy link
Collaborator Author

As a side note, can you @sherjeelshabih collect everything that we are cutting it somewhere? I can then add it to the docs later on.

Does this work: https://github.com/FAIRmat-NFDI/pynxtools/blob/6e5ef572dcbc02ed72ebda03edf86ca547ffceae/paper/paper.md

@mkuehbach
Copy link
Collaborator
mkuehbach commented Jul 1, 2025 •
edited by lukaspie
Loading

Assessed this https://github.com/FAIRmat-NFDI/pynxtools/actions/runs/16000718110/artifacts/3440976900

Title/Authors:

  • Full names should be displayed, currently only first names
  • Add Carlos, ask Carlos about his affiliation this is probably outdated, maybe also affilitation 1 is correct i.e. Physics, CSMB https://www.palmalab.org/contact.html
  • Full addresses for all affiliations missing
  • Do our affiliations have ROR keys? If so, use them https://joss.readthedocs.io/en/latest/paper.html#affiliations
  • Florian had a double affiliation when he was working for FAIRmat on pynxtools relevant that could be relevant to mention?
  • Marius Grundmann as an author missing who supported with in-kind contributions if I am not mistaken Carola and Ron

Summary:

  • "CLI" > "command line interface", i.e. remove abbreviation, better spell out upon first occurrence
  • should add https://www.nature.com/articles/s41597-025-04451-9 to the FAIR references
  • need to add more references to NeXus see full list of at least three papers in the NeXus paper (Koennecke 2015, Koennecke2006 10.1016/j.physb.2006.06.106, and Klosowski1997 10.1016/S0921-4526(97)00865-X)
  • list methods alphabetically
  • framing of the fragment via "and electronic lab notebook metadata" commas debatable, I would remove these
  • not primarily the data conversion is standardized to the NeXus format but the data model used
  • ", while performing" > "and performs validation to ensure ..."

Statement of need:

  • I am not convinced that "experimental physics" covers the theorical and computational subcommunities within the condensed-matter physics which are also though FAIRmat's target community, there I would edit "experimental physics"
    also I would add "materials science and materials engineering" to bridge to our colleagues of other NFDI consortia implicitly
  • maybe "but none offers a comprehensive" > "but no comprehensive"
  • Two whitespaces before "This approach"?
  • in line 33 and line 37 there is two times the point made that pynxtools makes working with NeXus accessible and between the lines easier, maybe a redundancy we could remove in one case, the sentence in line 40 again makes the point of making the complex easy, so indeed, the statement of need has at least one sentence too much here.

Dataconverter and validation:

  • ", core module of pynxtools," > ", the core module of,"
  • line51 "plugins distributed" > "plugins which are distributed"
  • line53 "It passes" > which is the specific noun "it" refers to? consider editing maybe
  • line62 "via GitHub CI" > "via GitHub continuous integration"
  • line63 "and integration across" > "and the integration across"

NeXus reader and annotator:

  • remove "read_nexus" from section title
  • line69 "concepts, reporting" > "concepts. The tool reports concept definitions, inheritance chains, and when data items are not mapped to a schema."
    I personally do not like that the all caps message NOT IN SCHEMA shows so prominently in the paper, I expect this in a documentation but not in a paper, see my suggestion the sentence before.
  • Move the sentence on plottable data down.
  • One sentence for each with the following priority order: 1. semantic use, 2. concept, 3. annotations (here one sentence for each), the remaining two parts documentation and default plottable thereafter, the current paragraph about read_nexus has gotten too short as we discussed yesterday.

NOMAD integration:

  • line78 references to NOMAD, spell the acronym NOMAD and repeat Scheidgen2023 refs, another one for the Scheffler/Draxl nature paper, maybe https://iopscience.iop.org/article/10.1088/2515-7639/ab13bb
    add the PSI-K Ghiringhelli paper on metadata and a link to the nomad webpage at the end of the sentence in line80, i.e. after "theory and experiment".
  • Consistence, I would remove all bullet points line81 and following and rather make each entry point an own short paragraph like done under Dataconverter,
  • NOMAD Metainfo should refer to Ghiringhelli paper, doi:10.1038/s41524-017-0048-5 on this subject
  • line84 "making NeXus conversion" > "making the conversion of data to NeXus accessible via the NOMAD GUI."
  • "dashboard for users, enabling them to efficiently filter" > "dashboard whereby users can efficiently filter"
  • not "and other relevant quantities" but "and other relevant quantities (specifically domain- and technique-specific ones)."
  • "upload shipped" > "upload that is shipped"

Author contributions:

  • Refer to the detailed github history or remove

References:

  • Check each reference carefully
  • each url needs date last visited
  • use et. al. to cut on too many authors, specifically as e.g. line141 "& others" is non standard
  • Behnel, use correct capitalization of the title https://lxml.de/capitalization, url, urldate, no more mentioning, for all urls - url and urldate last visited,
    • 8000
  • Hoyer is no longer in revision https://openresearchsoftware.metajnl.com/articles/10.5334/jors.148
  • Jacobsen doi missing
  • Könnecke doi misising
  • Larsen improper doi, no http links but this is the proper doi 10.1088/1361-648X/aa680e
  • why mentioning latest for github, plain links here point to what is then the latest
  • click needs proper author names
  • h5py needs proper author names
  • Wilkinson lacks doi

Final checks:

@mkuehbach mkuehbach self-requested a review July 1, 2025 20:08
mkuehbach
mkuehbach requested changes Jul 1, 2025
View reviewed changes
Copy link
Collaborator
@mkuehbach mkuehbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See last comment

sanbrock
sanbrock requested changes Jul 2, 2025
View reviewed changes
@sherjeelshabih
Copy link
Collaborator Author
sherjeelshabih commented Jul 6, 2025 •
edited
Loading
  • each url needs date last visited

I don't think that is necessary these days. It looks ugly as well.

  • click needs proper author names
  • h5py needs proper author names

I couldn't find a specific list to go for. They mention it like this as well.

@lukaspie lukaspie force-pushed the joss_paper_dataconverter_edit branch from cae09f0 to b798467 Compare July 7, 2025 09:36
@lukaspie
Copy link
Collaborator
lukaspie commented Jul 7, 2025
  • each url needs date last visited

I don't think that is necessary these days. It looks ugly as well.

We will not add them, as discussed.

  • click needs proper author names
  • h5py needs proper author names

I couldn't find a specific list to go for. They mention it like this as well.

Found and added better ones.

@lukaspie lukaspie requested a review from mkuehbach July 7, 2025 10:09
mkuehbach
mkuehbach approved these changes Jul 7, 2025
View reviewed changes
Copy link
Collaborator
@mkuehbach mkuehbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Points were addressed to satisfaction.

@lukaspie lukaspie merged commit 7f841fc into joss_paper Jul 7, 2025
2 checks passed
@lukaspie lukaspie deleted the joss_paper_dataconverter_edit branch July 7, 2025 10:10
@lukaspie lukaspie mentioned this pull request Jul 7, 2025
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaspie lukaspie lukaspie left review comments

@sanbrock sanbrock sanbrock requested changes

@mkuehbach mkuehbach mkuehbach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sherjeelshabih @mkuehbach @lukaspie @sanbrock
0