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

TEST: comparison between our current state and the new fairmat-2024 branch #289

Closed
wants to merge 84 commits into from

Conversation

lukaspie
Copy link
Collaborator

Comparison after merging nexusformat/definitions/main into the fairmat_2024 (which originally tracked the current state of fairmat)

woutdenolf and others added 30 commits June 12, 2023 17:22
Contributor links were not closed causing the next anchor to point elsewhere. This fixes nexusformat#1301
the value of a directional NXtransformations field is ignored
…ame_indices-confusion

NXdata: improve documentation regarding axes
…-action-update

update GHA checkout to current version
* Add new datatype, the union of NX_CHAR and NX_NUMBER
* Change AXISNAME field of NXdata to allow character values
* Add NXdata attribute default_slice: which slice of data to show in a plot by default. This is useful especially for datasets with more than 2 dimensions.

Co-authored-by: Sophie Hotz <[email protected]>
Add NXdetector_channel as a base class

Also add it as optional for NXmx NXdetector

See original Dectris proposal:
https://github.com/dectris/documentation/tree/main/filewriter_v2#user-content-multi-channel-data

Closes nexusformat#940

Co-authored-by: Sophie Hotz <[email protected]>
…port clarification, and file extension handling
@@ -3,7 +3,7 @@
<!--
# NeXus - Neutron and X-ray Common Data Format
#
# Copyright (C) 2008-2022 NeXus International Advisory Committee (NIAC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

NXevent_data has not been created 2008 but much later, so all copyright header for all definitions always the entire history? or history as per xml file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from NIAC, I have kept it here and in all other existing nxdl files.

<attribute name="short_name">
<doc>short name for instrument, perhaps the acronym</doc>
</attribute>
</field>
<group type="NXactuator"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I see this large collection of ALLCAPITAL groups that one can add to an instrument but most of these groups are in the base class just mentioned. Definitions made in base classes are optional to take effect these groups need to be specified in the appdefs anyhow why then at all to add ALLCAPITAL other base classes into a base class if these are not modified in any way?

I can understand the idea to e..g sugfgest when you have an instrument it may be worthwhile talking about its components but why to list them then here if these are ALLCAPITAL in my opinion this should be another PR to remove and clean NeXus of this rather suggestive nature what else users may need/wish to talk about.

The situation becomes different when one states in a base class that one such group gets mixed or all lowercase name but this is not the case in many instances, NXinstrument is the most prominent example. I guess this is still legacy of that NeXus comes from beam lines and THE instrument is the might thing but think further

NXmicrostructure what else could be interesting to talk about here (NXthermodynamical_properties) as an all capital suggestion but without any further detailing in the NXmicrostructure base class...? This should be avoided IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also why having to maintain a list of all potential NXcomponents that you could hook to an instrument, removing these would make the base classes cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is all heritage from NIAC. I think it's good to already define these in the base class NXinstrument because then the appdefs don't actually have to add them on top.

@@ -2,9 +2,9 @@
<?xml-stylesheet type="text/xsl" href="nxdlformat.xsl"?>
<!--
# NeXus - Neutron and X-ray Common Data Format
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you see this inconsistency again with year timestamps what is de facto the license header, I thought the entire product NeXus definitions has the same license giving different numbers like 2008-2024, 2014-2024 is inconsistent see above comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the ones coming from our side. nyaml2nxdl basically just writes 2014 as the default starting date. We could change this to 2008 though.

base_classes/NXmonochromator.nxdl.xml Show resolved Hide resolved
@@ -27,7 +27,7 @@
name="NXparameters"
type="group" extends="NXobject">
<doc>Container for parameters, usually used in processing or analysis.</doc>
<field name="term" minOccurs="0" maxOccurs="unbounded" type="NX_CHAR">
<field name="TERM" minOccurs="0" maxOccurs="unbounded" nameType="any">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from NIAC

base_classes/NXsample_component.nxdl.xml Show resolved Hide resolved
base_classes/NXwaveplate.nxdl.xml Outdated Show resolved Hide resolved
contributed_definitions/NXbeam_path.nxdl.xml Show resolved Hide resolved
contributed_definitions/NXregion.nxdl.xml Show resolved Hide resolved
@lukaspie lukaspie force-pushed the fairmat-2024 branch 2 times, most recently from eec8808 to 7284513 Compare September 20, 2024 12:17
black formatting

isort dev_tools
@lukaspie
Copy link
Collaborator Author

lukaspie commented Oct 8, 2024

Closed as discussions and updates will occur in the individual branches

@lukaspie lukaspie closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.