Skip to content
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

add identifierNAME to NXobject #1486

Merged
merged 16 commits into from
Jan 7, 2025

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Sep 30, 2024

Implements vote in #1451 (comment).
Depends on #1485 -> done

Any implementation should consider comments in #1416

@rayosborn
Copy link
Contributor

rayosborn commented Oct 4, 2024

I thought that, in the end, we decided against using "identiferNAME", instead of an identifier attribute, just called "identifier". Obviously, @sanbrock has the official record.

@prjemian
Copy link
Contributor

prjemian commented Oct 4, 2024

If it is an attribute, then it would be added to the nxdl.xsd schema.

@rayosborn
Copy link
Contributor

Isn't it possible also to add attributes to the NXobject class at least for groups?

@prjemian
Copy link
Contributor

prjemian commented Oct 4, 2024 via email

@lukaspie
Copy link
Contributor Author

lukaspie commented Oct 7, 2024

I added some of the feedback provided by @paulmillar in #1416 with respect to the type of identifier. He made a strong point that service is not the right word, but rather something like type.

Note that here I am still adding identifierNAME as a field rather than an attribute. My argument for this is that if we were to use an attribute and still keep the service and is_persistent (as was agreed if I recall correctly), we would need to add three attributes like this: identifierNAME, identifier_serviceNAME, identifier_is_persistentNAME, which would be rather messy. Since identifier will be added to the reserved prefixes anyway, why not make it a field with the other two concepts as attributes? Maybe @sanbrock can also chime in with his opinion.

I thought that, in the end, we decided against using "identiferNAME", instead of an identifier attribute, just called "identifier". Obviously, @sanbrock has the official record.

My understanding was that it shall be possible to have more than one identifier (e.g., from different services) for one object. I think this is also what @sanbrock noted. Even if identifier will become an attribute, I would suggest to use <attribute name="identifierNAME" type="NX_CHAR" nameType="partial">.

@prjemian
Copy link
Contributor

prjemian commented Oct 7, 2024

We have been reducing use of type to a few limited situations because its meaning varies with context. XML Schema, XML, NXDL, numeric, data, units, ... (I'm certain I've missed a few.) All of these want to use this noun. Quickly, it became bewildering. At the end of the day, the XML Schema and the NXDL data type win. Here's an example:

<xs:element name="definition" type="nx:definitionType">

Please pick a noun which is not type.

@rayosborn
Copy link
Contributor

At the NIAC, I recall that people (including, I believe, @phyy-nx) thought the "is_persistent" tag unnecessary. We need @sanbrock to confirm, but I also believe that the attributes would just be "identifier" and "service." There is no need to add partial names here, which I agree become very messy. I have no idea whether we specifically voted on any of these issues.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

We have not voted on it but agreed that a workgrouo shall conclud with a reasonable solution in a form of a PR which can then be voted on.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

Indeed, we qgreef to use type instead of service.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

There was no real conclusion on is_persistant. Indeed, its necessity was debated, although I argued that it is acually good to know if an Idebtifier is a PID or not.
Ww may state that the type holds this information. E.g. doi or orcid are good examples of PIDs.

@rayosborn
Copy link
Contributor

@sanbrock, do you believe we came to any conclusion about the need for partial names? Attribute names such as "identifierDOI" are pretty ugly IMHO. If there is going to be a working group, perhaps this PR should be, at least temporarily, withdrawn.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

Note that we did agree that our solution shall allow attaching multiple identifiers (e.g. orcid, linkdin to a USER).

  • identifierNAME with nameType=partial would do that job.
  • identifier as a reserved prefix would also do that (but this solution is rather a convention /only appears in the documentation and/or in the xsd descriptuon of the NeXus Definiion Language/ rather than an asserted statement written in NXDL in a definition. While the former requires hard coded implementation in all NEXUS tools in any programming languages used, the later would be inferred automatically by any NEXUS tools which has implemented the interpretation of NeXus definitions written in NXDL. Because of this, I would prefer @identifierNAME with @identifier_typeNAME declared in NXobject over the actual simpler solution of listing identifier among the reserved prefixes.
  • identifierNAME as a deckared Field in NXobject would also do the same, but it would allow adding @type to it. This seems to be the most elegant solution. Knowing that only a few udentifiers will be provided in practice for a given group, I do not think it has disadvantages compared to using attributes.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

@sanbrock, do you believe we came to any conclusion about the need for partial names? Attribute names such as "identifierDOI" are pretty ugly IMHO. If there is going to be a working group, perhaps this PR should be, at least temporarily, withdrawn.

This is actually a draft PR. And we are the workgroup. Note that we wanted to invite @paulmillar too to this duscussion.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

Note that another alternative is
@identifierTYPE - this allows the use of identifier_orcid, identifier_url, identifier_iri, etc. in the data file, and would make the @identifier_typeNAME (or identifier/@type) as an extra attribute unnecessary.

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

NIAC suggestions on the revwrite of NXidentifirler were recorded in #1451

@sanbrock
Copy link
Contributor

sanbrock commented Oct 7, 2024

These have been also voted for in Session J. See https://www.nexusformat.org/content/NIAC2024_minutes/

base_classes/NXobject.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXobject.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXobject.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXobject.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXobject.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXobject.nxdl.xml Show resolved Hide resolved
manual/source/datarules.rst Show resolved Hide resolved
@lukaspie lukaspie force-pushed the identifier-in-nxobject branch from f7e3efd to 95bdb03 Compare October 16, 2024 08:32
@lukaspie
Copy link
Contributor Author

CI/CD failing because multi-line doc handling is not implemented (see #1491)

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 7, 2024

While a separate page can be added on the use of Identifiers, during the discussion in the Telco it was also appreciated that the extensive documentation string is indeed collapsed in the HTML webpage.

Totally forgot. In that case I agree this can go to a vote in our next telco on Monday, once @paulmillar's new comments are addressed. We'll follow the normal procedure of kicking off a two week voting period, unless we happen to get quorum!

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 9, 2024

@lukaspie please remove type_other and Other from type (see comment on #1407). Once done, as discussed on the Telco, we will start an online vote for this PR. Thanks!

@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 9, 2024

@lukaspie please remove type_other and Other from type (see comment on #1407). Once done, as discussed on the Telco, we will start an online vote for this PR. Thanks!

done!

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 10, 2024

Hi all, as discussed in the Telco, now that the above points have been addressed (thanks @lukaspie), we can now move this PR to an online vote.  NIAC committee members please vote on this PR using emojis. 👍 for yes, 👎 for no, anything else (for example 👀) to abstain.  We need 14 votes to hit quorum so please review and vote!

@phyy-nx phyy-nx mentioned this pull request Dec 10, 2024
2 tasks
@lukaspie lukaspie added the NIAC vote needed PR needs an approving vote from NIAC before merge label Dec 11, 2024
lukaspie and others added 3 commits January 6, 2025 10:59
Co-authored-by: Peter Chang <peter.chang@diamond.ac.uk>
Copy link
Contributor

@PeterC-DLS PeterC-DLS left a comment

Choose a reason for hiding this comment

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

LGTM

@lukaspie lukaspie added NIAC approved and removed NIAC vote needed PR needs an approving vote from NIAC before merge labels Jan 6, 2025
@phyy-nx
Copy link
Contributor

phyy-nx commented Jan 6, 2025

Vote has passed with 16 votes. Thanks all!

@lukaspie feel free to merge

@lukaspie
Copy link
Contributor Author

lukaspie commented Jan 7, 2025

Vote has passed with 16 votes. Thanks all!

@lukaspie feel free to merge

Great, thanks!

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

Successfully merging this pull request may close these issues.

7 participants