FAIRmat-NFDI / nexus_definitions

Definitions of the NeXus Standard File Structure and Contents
https://manual.nexusformat.org/
Other
5 stars 8 forks source link

Use @target attribute in NXresolution #84

Closed domna closed 8 months ago

domna commented 8 months ago

Currently, NXresolution uses its own NXlink base class. However, this is the same concept as the nexus @target attribute.

lukaspie commented 8 months ago

@domna what was your suggestion here?

Currently, we have

  formula_symbol_LINK(NXlink):
    doc: |
      A symbol linking to another path in this appdef
      to be referred to from the `resolution_formula` field.
    symbol:
      exists: optional
    path:
      exists: optional
  resolution_formula:
    doc: |
      A resolution formula to determine the resolution from a set of symbols as
      entered by the `formula_symbol_...` fields.
      The output unit should match the provided unit of this field.

NXlink(NXobject):
  symbol:
    doc: |
      The symbol by which to refer to this link, e.g., for formulas
  path:
    doc: |
      A valid path inside an instance of an application definition, i.e.,
      of the form /entry/instrument/my_part/my_field

Was your plan to do it like this?

  formula_symbol_LINK(NX_CHAR):
    doc: |
      A symbol linking to another path in this appdef
      to be referred to from the `resolution_formula` field.
    symbol:
      exists: optional
    \@target:
      doc: |
        A valid path inside this application definition, i.e.,
        of the form /entry/instrument/my_part/my_field
      exists: optional
  resolution_formula(NX_CHAR):
    doc: |
      A resolution formula to determine the resolution from a set of symbols as
      entered by the `formula_symbol_...` fields.
      The output unit should match the provided unit of this field.
lukaspie commented 8 months ago

I am using branch 84-use-target-attribute-in-nxresolution for this issue.

domna commented 8 months ago

Yes this was my intention. Then we can remove the NXlink base class. Maybe we should also set the symbol and \@target to required, otherwise the link itself doesn't make too much sense (we could also think of adding a description of the symbol instead of a link). Also the symbol can be removed because we can insert it directly into the field value. So this is how it would look like:

formula_symbol_LINK(NX_CHAR):
    doc: |
      A symbol linking to another path in this appdef
      to be referred to from the `resolution_formula` field.
    \@target:
      doc: |
        A valid path inside this application definition, i.e.,
        of the form /entry/instrument/my_part/my_field
  resolution_formula(NX_CHAR):
    doc: |
      A resolution formula to determine the resolution from a set of symbols as
      entered by the `formula_symbol_...` fields.
      The output unit should match the provided unit of this field.
lukaspie commented 8 months ago

Ok, making them required sounds good to me. However, I am not sure we are using the @target attribute correctly. In our case, the yaml looks like this:

formula_symbol_LINK(NX_CHAR):
  doc: |
    A symbol linking to another path in this appdef
    to be referred to from the `resolution_formula` field.
  \@target:
    doc: |
      A valid path inside this application definition, i.e.,
      of the form /entry/instrument/my_part/my_field

resulting in this nxdl like this:

<field name="formula_symbol_LINK" type="NX_CHAR">
    <doc>
        A symbol linking to another path in this appdef
        to be referred to from the `resolution_formula` field.
    </doc>
    <attribute name="target">
        <doc>
            A valid path inside this application definition, i.e.,
            of the form /entry/instrument/my_part/my_field
        </doc>
    </attribute>
</field>

But, if you look at how other people have been using target (NeXuS docs), it looks like this:

<group type="NXdata">
    <link name="polar_angle" target="/NXentry/NXinstrument/NXdetector/polar_angle">
        <doc>Link to polar angle in /NXentry/NXinstrument/NXdetector</doc>
    </link>
    <link name="data" target="/NXentry/NXinstrument/NXdetector/data">
        <doc>Link to data in /NXentry/NXinstrument/NXdetector</doc>
     </link>

which converts to nyaml like this:

(NXdata):
  polar_angle(link):
    target: /NXentry/NXinstrument/NXdetector/polar_angle
    doc: |
      Link to polar angle in /NXentry/NXinstrument/NXdetector
  data(link):
    target: /NXentry/NXinstrument/NXdetector/data
    doc: |
      Link to data in /NXentry/NXinstrument/NXdetector

In that case, they are hard-coding in the app-def that polar_angle must always be a link to /NXentry/NXinstrument/NXdetector/polar_angle. However, what we would like to do is give the option of adding a link at creation of the file. Do you think it still works like this?

domna commented 8 months ago

Ok, this is indeed different than what I initially thought @target is for. I understood from here that this is just a normal attribute we can insert any hdf5 path into. But it also seems to define the type of the target in the appdef, which is closer to the sameAs property we thought of.

~But it seems NeXus contains two ideas of a target, the one your describe is this one: https://github.com/nexusformat/definitions/blob/df84535e6b053e7c8c17d2ff6d732d3dca4d879f/nxdl.xsd#L544C5-L544C5~

~the one which is described in the documentation I linked to is this one: https://github.com/nexusformat/definitions/blob/df84535e6b053e7c8c17d2ff6d732d3dca4d879f/nxdl.xsd#L1045C12-L1045C12~

~I think here we want to use the @target attribute (like you initially wrote it). The other one we might be able to use for the sameAs property.~

No, sorry this is the same concept. It's just the description of the target xml attribute and not the description of an nxdl attribute. So it seems we only have the option to specify a target of a specific nexus concept. I don't know if we can just say NXObject here to declare that we don't care what the link target is!? Or we stick with the NXlink base class.

lukaspie commented 8 months ago

I guess NXlink may be problematic on its own, since it is used as a class in the NAPI and in nexpy. I know both tools are not part of the official NeXus Standard, but it is probably not ideal to interfere with these existing tools.

Not sure if it works with NXobject, I guess one would write it like this:

formula_symbol_LINK(link):
  doc: |
    A symbol linking to another path in this appdef
    to be referred to from the `resolution_formula` field.
  target: NXobject

translating to

<link name="formula_symbol_LINK" target="NXobject">
    <doc>
        A symbol linking to another path in this appdef
        to be referred to from the `resolution_formula` field.
    </doc>
</link>

I don't know if you can then change the link when you create the file.

domna commented 8 months ago

I guess NXlink may be problematic on its own, since it is used as a class in the NAPI and in nexpy. I know both tools are not part of the official NeXus Standard, but it is probably not ideal to interfere with these existing tools.

Ok, so if we would use this NXlink base class we should at least rename it to something else (I never checked if there are any name collisions as this was just a rough draft anyways).

Not sure if it works with NXobject, I guess one would write it like this:

formula_symbol_LINK(link):
  doc: |
    A symbol linking to another path in this appdef
    to be referred to from the `resolution_formula` field.
  target: NXobject

translating to

<link name="formula_symbol_LINK" target="NXobject">
    <doc>
        A symbol linking to another path in this appdef
        to be referred to from the `resolution_formula` field.
    </doc>
</link>

I don't know if you can then change the link when you create the file.

I think that we don't have any support for the link type in pynxtools anyways. So we could just implement it such that the target allows for the class itself and its subclasses. Therefore, NXobject should allow to write every nexus class. I think this is also in accordance how nexus inheritance works (or at least how we decided to use it).

@sherjeelshabih Are you aware of an implementation of the link property (like my_field(link)) in pynxtools?

If we don't support anything like this we should also add a backlog issue if we decide to use the link type here.

sherjeelshabih commented 8 months ago

I guess NXlink may be problematic on its own, since it is used as a class in the NAPI and in nexpy. I know both tools are not part of the official NeXus Standard, but it is probably not ideal to interfere with these existing tools.

Ok, so if we would use this NXlink base class we should at least rename it to something else (I never checked if there are any name collisions as this was just a rough draft anyways).

Not sure if it works with NXobject, I guess one would write it like this:

formula_symbol_LINK(link):
  doc: |
    A symbol linking to another path in this appdef
    to be referred to from the `resolution_formula` field.
  target: NXobject

translating to

<link name="formula_symbol_LINK" target="NXobject">
    <doc>
        A symbol linking to another path in this appdef
        to be referred to from the `resolution_formula` field.
    </doc>
</link>

I don't know if you can then change the link when you create the file.

I think that we don't have any support for the link type in pynxtools anyways. So we could just implement it such that the target allows for the class itself and its subclasses. Therefore, NXobject should allow to write every nexus class. I think this is also in accordance how nexus inheritance works (or at least how we decided to use it).

@sherjeelshabih Are you aware of an implementation of the link property (like my_field(link)) in pynxtools?

If we don't support anything like this we should also add a backlog issue if we decide to use the link type here.

There is an HDF5 link implementation in pynxtools. We can always use that for linking any field in the same or other AppDef entry in an HDF5 file.

I was going through the thread. And I feel that what you need is the regular NXlink already in NeXus. When I went through the docs I was a bit confused. We can discuss this on Monday after the DST to clarify what NXlink exactly is intended for.

lukaspie commented 8 months ago

Rename to formula_SYMBOL

formula_SYMBOL(NX_CHAR):
  doc: |
    Combine symbol+path docstring
lukaspie commented 8 months ago

To summarize, we agreed to go with:

NXresolution(NXobject):
  ...
  formula_SYMBOL(NX_CHAR):
    doc: |
      A symbol linking to another path in this appdef to be referred to from the 
      `resolution_formula` field. This should be a valid path inside this application
      definition, i.e., of the form /entry/instrument/my_part/my_field    
  resolution_formula(NX_CHAR):
    doc: |
      A resolution formula to determine the resolution from a set of symbols as
      entered by the `formula_...` fields.
      The output unit should match the provided unit of this field.

and remove NXlink base class.

Do you agree with this @domna? If so, I will merge everything into #52 .

domna commented 8 months ago

Yes, sounds perfect 👍️

lukaspie commented 8 months ago

Closed with changes merged into #52