Skip to content

Conversation

@lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Sep 23, 2024

Here, several new fields are added to NXentry, to give a more detailed of where (experiment_{location,facility,institution} ) and when (experiment_{start_date,end_date}) the experiment was performed.

@lukaspie lukaspie marked this pull request as draft September 23, 2024 15:06
@lukaspie lukaspie force-pushed the fairmat-2024-nxentry branch from ff097f7 to e0b24a6 Compare September 23, 2024 15:13
@lukaspie lukaspie marked this pull request as ready for review September 23, 2024 15:15
This was referenced Sep 29, 2024
This was linked to issues Sep 29, 2024
@woutdenolf
Copy link
Contributor

Do we know what these are identifiers of? There is an issue about it: #1043

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 29, 2024

Instead of this, add PID to NXobject as an attribute.

@woutdenolf
Copy link
Contributor

Will a group ever need more than one PID?

@lukaspie lukaspie changed the title Fairmat 2024: introduce NXidentfier, use in NXentry and NXsubentry Fairmat 2024: introduce NXidentifier, use in NXentry and NXsubentry Oct 7, 2024
@lukaspie lukaspie changed the title Fairmat 2024: introduce NXidentifier, use in NXentry and NXsubentry Fairmat 2024: new fields for experiment description in NXentry and NXsubentry Oct 16, 2024
@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 6, 2024

Note that previous versions of this PR contained changes to the *_identifier fields in NXentry to make use of the then-proposed new base class NXidentifier. Currently, all discussion of identifiers has been removed.

@phyy-nx @sanbrock since there has anyway been no vote, I would suggest we wait with this PR until the discussion around #1486 has been settled and then make the necessary changes here on top afterwards.

@phyy-nx
Copy link
Contributor

phyy-nx commented Feb 4, 2025

@lukaspie since #1486 has been merged is this one ready for discussion again?

@lukaspie
Copy link
Contributor Author

lukaspie commented Feb 5, 2025

@lukaspie since #1486 has been merged is this one ready for discussion again?

We were thinking about replacing the existing identifiers with fields starting with identifierNAME. For example, adding an identifier_entry and deprecating entry_identifier. With the idea that all identifiers in NeXus should follow this general idea of starting with identifier.... I haven't implemented this yet though. Would you think that this is a good way forward? If so, I can implement it and this PR would be ready to be discussed. All other changes are ready anyway.

@phyy-nx
Copy link
Contributor

phyy-nx commented Feb 5, 2025

Seems reasonable to me.

@lukaspie lukaspie force-pushed the fairmat-2024-nxentry branch 2 times, most recently from 23ce833 to 0097b28 Compare February 6, 2025 15:20
@lukaspie
Copy link
Contributor Author

lukaspie commented Feb 6, 2025

Seems reasonable to me.

I made the necessary changes and deprecated the ..._identifier fields. Probably warrants some discussion whether all of these should be kept in the new identifier_... format, but this PR should be ready to be discussed now.

domna and others added 8 commits February 6, 2025 16:22
… version of yaml.

Removing unintensional comments
* Add appdef/bc yaml files to gitignore

* Improve Makefile for nxdl build

* Tricking make to avoid circular dependency

* Generalization and cleanup

* Change chmod 755 to 644 for NXentry

* Reintroduce for-loop for make nyaml

* Adds push to github pages

* Set correct docs dir

* Prepare building

* Change id to name

* Build and deploy

* Push pages to fairmat-docs

* Adds clean flag

* Use docs folder

* Target fairmat branch to rebuild live-docs

* Removes old workflows

* Updates color and logo

* Optimise styling

* Add logo padding

* Run ci on pr

* Adds cleanup ci

* Removes yaml files from .gitignore
# Conflicts:
#	.gitignore
#	Makefile
#	manual/source/_static/to_alabaster.css
#	manual/source/img/FAIRmat_new.png
# Conflicts:
#	base_classes/NXsubentry.nxdl.xml
#	base_classes/nyaml/NXentry.yaml
#	base_classes/nyaml/NXsubentry.yaml
#	contributed_definitions/NXsts.nxdl.xml
#	contributed_definitions/nyaml/NXsensor_scan.yaml
#	contributed_definitions/nyaml/NXsts.yaml
#	contributed_definitions/nyaml/NXtransmission.yaml
…ersion

# Conflicts:
#	applications/NXarpes.nxdl.xml
#	applications/nyaml/NXarpes.yaml
#	base_classes/NXaperture.nxdl.xml
#	base_classes/NXbeam.nxdl.xml
#	base_classes/NXdata.nxdl.xml
#	base_classes/NXdetector.nxdl.xml
#	base_classes/NXenvironment.nxdl.xml
#	base_classes/NXinstrument.nxdl.xml
#	base_classes/NXmonochromator.nxdl.xml
#	base_classes/NXroot.nxdl.xml
#	base_classes/NXsample.nxdl.xml
#	base_classes/NXsample_component.nxdl.xml
#	base_classes/NXsensor.nxdl.xml
#	base_classes/NXsource.nxdl.xml
#	base_classes/NXsubentry.nxdl.xml
#	base_classes/NXtransformations.nxdl.xml
#	base_classes/NXuser.nxdl.xml
#	base_classes/nyaml/NXaperture.yaml
#	base_classes/nyaml/NXbeam.yaml
#	base_classes/nyaml/NXdata.yaml
#	base_classes/nyaml/NXdetector.yaml
#	base_classes/nyaml/NXentry.yaml
#	base_classes/nyaml/NXenvironment.yaml
#	base_classes/nyaml/NXinstrument.yaml
#	base_classes/nyaml/NXmonochromator.yaml
#	base_classes/nyaml/NXprocess.yaml
#	base_classes/nyaml/NXroot.yaml
#	base_classes/nyaml/NXsample.yaml
#	base_classes/nyaml/NXsample_component.yaml
#	base_classes/nyaml/NXsensor.yaml
#	base_classes/nyaml/NXsource.yaml
#	base_classes/nyaml/NXsubentry.yaml
#	base_classes/nyaml/NXtransformations.yaml
#	base_classes/nyaml/NXuser.yaml
@phyy-nx
Copy link
Contributor

phyy-nx commented Aug 11, 2025

From Telco 08/11/25: NXentry@experiment_identifier seems baked deep into the DNA of NeXus, so deprecating it in favor of NXobject@identifierNAME could cause some problems. Would welcome feedback from the NIAC here.

@phyy-nx phyy-nx added this to the NXDL 2026 milestone Aug 11, 2025
@phyy-nx
Copy link
Contributor

phyy-nx commented Aug 11, 2025

Alternatively, we could change NXobject@identiferNAME to NXobject@PREFIXidentifierSUFFIX, which would allow the use of experiment_identifer as is. But that would lock away the string 'identifer' entirely, such that it could never be used by a user.

@phyy-nx phyy-nx modified the milestones: NXDL 2026, NXDL 2025 Aug 11, 2025
@lukaspie lukaspie added the NIAC vote needed PR needs an approving vote from NIAC before merge label Aug 25, 2025
@phyy-nx
Copy link
Contributor

phyy-nx commented Aug 25, 2025

From Telco 08/25/25:

Two proposals:

  1. Change NXobject/identifierNAME to NXobject/PREFIXidentifierSUFFIX.
  2. Add notes to NXentry/XXX_identifier that these should be considered identifiers as described in NXobject/identifierNAME

Regardless, we shouldn't deprecate NXentry/XXX_identifier

Pros/cons for 1.: simpler but locks away the word 'identifer' forever
Pros/cons for 2.: no functional change but complicates validation

Can I get a straw poll here on the preferred method? Do you prefer 1. over 2.? Vote 👍 for 1. and 👎 for 2.

@phyy-nx
Copy link
Contributor

phyy-nx commented Aug 29, 2025

Moar votes on the straw poll pls? :)

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 16, 2025

NXarchive uses the same convention of experiment_identifier, collection_identifier, and entry_identifier, which terms are expected by ICAT. This is another argument against deprecation.

Proposal from the telco: nearly all of the identifierNAME convention from #1486 is new, but all the instances of NAMEidentifier (e.g. experiment_identifier in NXentry is old. Therefore since there hasn't been a release yet, it would be doable to swap to NAMEidentifier as the convention.

This would change new application definitions such as NXem like this:

<field name="identifierNAME" type="NX_CHAR" nameType="partial" recommended="true">

Becomes

<field name="NAMEidentifier" type="NX_CHAR" nameType="partial" recommended="true">

And

<field name="identifier_sample" type="NX_CHAR" recommended="true">

becomes

<field name="sample_identifier" type="NX_CHAR" recommended="true">

This would also alter NXobject which describes the identifierNAME convention, changing it to NAMEidentifier.

Can I get a straw poll on this idea here? Vote 👍 or 👎 on this comment for if you agree to go from identifierNAME to NAMEidentifier.

@rayosborn
Copy link
Contributor

Does the new NAMEidentifier partial name permit the use of an underscore? Or would NAME_identifier be better? I think the NeXus convention has been to split portmanteau names with underscores, so I was always a little uncomfortable with identifierNAME but went along because I think I was late to the discussion.

@mkuehbach
Copy link
Contributor

mkuehbach commented Sep 16, 2025

RE: @phyy-nx @sanbrock
Wrt to these last parts of the discussions on this PR thread, discussed at the 2025/09/16 telco:

  • (and other allowed symbols, such as ``_``) are not.
    states that _ is not replaceable hence NAME_identifier will force folks into having to accept concepts with a leading _ dangling always even if the NAME gets dropped, I cannot vote for it official but I am against it. Does any concept's name in NeXus right now starts with _ ? ... so, if that change is required, NAMEidentifier should be it.
  • Wrt to when to add underscores the documentation is clear [https://manual.nexusformat.org/applying-nexus.html](separate multi worded names with underscores).
  • Assuming that the straw poll above turns out such that it supports to convert the above request into a proper PR changing NX{em,apm,object,...} and alike: Is then one PR sufficient to change all FAIRmat appdefs and does this need a vote, i.e., how to proceed and when is the deadline of that straw poll for us to work effectively?

@mkuehbach
Copy link
Contributor

Will a group ever need more than one PID?

Not necessarily required but the situation a thing has not always exactly only one PID globally assigned might happen in the wild

@rayosborn
Copy link
Contributor

  • (and other allowed symbols, such as ``_``) are not.

    states that _ is not replaceable hence NAME_identifier will force folks into having to accept concepts with a leading _ dangling always even if the NAME gets dropped,

@mkuehbach, I'm a little confused by this statement. Are you saying that NAME_identifier is unacceptable because it would allow _identifier as a valid name? I don't think that can be true. A partial name implies that there must be something substituting for the placeholder, i.e., NAME; it can't be blank. Perhaps I misunderstood the comment.

@mkuehbach
Copy link
Contributor

mkuehbach commented Sep 17, 2025

(empty string allowed) and the lower case letters

"(empty string allowed)" means NAME_identifier allows for _identifier. NAME can be blank.

@lukaspie
Copy link
Contributor Author

In the discussion around identifierNAME, it was explicitly stated that it is permitted to use identifier, i.e., replacing the placeholder part NAME with an empty string. As @mkuehbach pointed out, this is defined in the nxdl.xsd as well:

(empty string allowed) and the lower case letters

I would strongly suggest to keep this convention because if there is a single identifier, it just makes sense to call it identifier and not add something to its name. Note that we have been operating based on this logic for a while now and would need to change a lot of the NeXus files that we have been creating.

Changing to NAMEidentifier is fine for me, but this will require quite a big change to now-accepted application definitions that have been making use of the inherited identifierNAME from NXobject. Guidance from NIAC how to implement this (along Markus' questions above) would be needed.

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 23, 2025

New proposal, we can't get consensus here. So @lukaspie simply remove the deprecations from this pull request and we'll vote on the new terms.

@lukaspie
Copy link
Contributor Author

New proposal, we can't get consensus here. So @lukaspie simply remove the deprecations from this pull request and we'll vote on the new terms.

done

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 24, 2025

Ok, this PR is now ready for a vote. Please vote using emojis, 👍 for yes, 👎 for now, and anything else e.g. 👀 for abstain. Voting will close in 2 weeks. Thanks!

@benajamin
Copy link
Contributor

I have a problem with the "experiment_location" field. The doc string says: "City and country where the experiment took place", which means that 2 pieces of information are intended for a single field. I would assume that it is intended to be a single comma separated list of "city, country" but we should not be in the business of making assumptions!

A separate issue is why these fields are being placed in NXentry, rather than NXroot? It does not make sense to me for such things to vary between different NXentry's in the one file.

@mkuehbach
Copy link
Contributor

On the first point I suggest to make NXresearch_project(NXnote) there one could have single fields city, country, geolocation, like for NXuser. Then in NXentry one would hook a group research_project but agree but yay maybe better to move this idea to an issue to the NXDL2026 milestone and instead now add city, country this is already good, geolocation I find better but there are different use cases with different location precision demands.

On the issue with assuming that each NXentry in a file was generated at the same location. Admitted this is a frequent case where multiple methods are combined at the same beamline or lab but what if I use a NeXus file to bundle such information for an interdisciplinary team where the sample was physically shipped and indeed the measurements or computer simulations/analyses where done in different places. Pinning that information hard into the root does no longer cleanly support such use case.

@benajamin
Copy link
Contributor

I don't think we need to go so far as to completely change the field into an NXresearch_project and would rather suggest adding to the doc string that the value of the field should be a single string comprised of the city, a comma and then the country.

I do agree that someone might want to append an NXentry to an existing file after moving an experiment to a new location (perhaps for a complementary technique), thus making a location described in the root inaccurate. However, this would be considered by many to be poor practice since data files should not be altered after a measurement is concluded. In the given example, better practice would be to have separate NeXus files for the measurements at different locations, and if a combined file is desired, then a new meta-file should be created that links to the relevant data files. I don't see any good reason why such a meta-file should include location information.

@paulmillar
Copy link

Sorry, I'm probably simply missing something, but I don't understand the motivation of this pull request.

Certainly, the information is worth recording. However, we already have NXInstrument that identifies the beamline, and NXsource that identifies the facility.

To me, it looks like the new fields would be duplicating information available through existing base classes.

Some additional thoughts...

If we want to record geographical location (a reasonable thing to do), please define how this information is recorded.

For example, we could specify that country is recorded using ISO 3166-1 (specifying whether A2 or A3 is acceptable). Germany would be "DE" (ISO-3166-1 A2) or "DEU" (ISO-3166-1 A3).

For cities, there's geonames. For Hamburg the GeoNames ID 2911298. City names can be ambiguous, even within a single country.

Another possible route would be to use persistent identifiers (PID), instead of using textual information, when describing this information. Using PIDs would have higher fidelity, but requires the software to do a metadata lookup to get information (name, geographic location, etc) about the target. The good part is that support for PIDs is already available in NeXus.

Here are possible PIDs for the different targets:

  • for organisation, there's ROR; e.g., DESY ... plus some others.
  • for beamlines, there's PIDINST / DOIs.
  • we currently lack a PID for facilities, but there's an RDA working group looking into this.

@phyy-nx
Copy link
Contributor

phyy-nx commented Oct 9, 2025

The vote has not passed, and additional discussion is needed. We invite the participants to join our next Telco, which will be advertised on the nexus-committee mailing list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NIAC vote needed PR needs an approving vote from NIAC before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NXentry NXsubentry

9 participants