implement depends_on - Githubissues

stuartcampbell commented 10 years ago

Original reporter: prjemian

see: http://wiki.nexusformat.org/Main_Page/NXdetector_2012_10 also see #112, #247, #268, and [1168] used in (amongst others): NXtransformations, NXmx

From Tobias: depends_on has crept into a number of places in the documentation now. My short summary:

It does exist both as a field for placing components (like NXdetector) and as an attribute to chain together transformations. A depends_on of "." is the explicit (and required) way of depending on the origin of the coordinate system.

stuartcampbell commented 10 years ago

Original reporter: prjemian

We need to decide whether this is an attribute or a field and use just one of them. On the wiki page (above), NXdetector_element, is used both ways. Looking at how it applies in all cases shown on the wiki, it should be an attribute of both groups and fields rather than defined as a field.

There are obvious advantages to defining "depends_on" as an attribute:

it is obvious to which parent it applies
only one instance is allowed per parent

stuartcampbell commented 10 years ago

Original reporter: prjemian

Please describe how the chain of values for depends_on is used. An example might be the clearest way. As it is written, it seems that a value of "." is the final element in the chain, not the first.

Also, to ensure that the depends_on chain still works with linked fields, the value of depends_on should be an absolute HDF5 address, not an address relative (such as without any path) to the instance.

stuartcampbell commented 10 years ago

Original reporter: zjttoefs

That's all correct. I can provide a link to an example in due course.

stuartcampbell commented 10 years ago

Original reporter: zjttoefs

Here's one:

http://www.ccp4.ac.uk/xia/thau_1.nxs

prjemian commented 9 years ago

Thanks, that helps. Next comment shows the structure of that HDF5 file.

prjemian commented 9 years ago

h5ToText.exe thau_1.nxs
thau_1.nxs : NeXus data file:NXroot
  @NX_class = NXroot
  @creator = CBFlib
  @creator_version = 0.9.5 (r492)  2014-04-27 17:00:40 +0100 (Sun, 27 Apr 2014)
  entry:NXentry
    @NX_class = NXentry
    definition:NX_CHAR = NXmx
      @version = 1.0
    end_time:NX_CHAR = 2014-04-25T09:34:06.087
    start_time:NX_CHAR = 2014-04-25T09:33:43.637
    data:NXdata
      @NX_class = NXdata
      @signal = data
      @axes = ['' 'y' 'x']
      @y_indices = 1
      @x_indices = 2
      data:NX_INT32[450,2527,2463] = __array
        __array = [
            [
                [-2, 0, 0, '...', -2]
                [0, 0, 0, '...', 0]
                [0, 0, 0, '...', 0]
                ...
                [-2, -2, -2, '...', -2]
              ]
            [
                [-2, 0, 0, '...', -2]
                [0, 0, 0, '...', 0]
                [0, 0, 0, '...', 0]
                ...
                [-2, -2, -2, '...', -2]
              ]
            [
                [-2, 0, 0, '...', -2]
                [0, 0, 0, '...', 0]
                [0, 0, 0, '...', 0]
                ...
                [-2, -2, -2, '...', -2]
              ]
            ...
            [
                [-2, 0, 0, '...', -2]
                [0, 0, 0, '...', 0]
                [0, 0, 0, '...', 0]
                ...
                [-2, -2, -2, '...', -2]
              ]
          ]
        @signal = 1
      x:NX_FLOAT64[2463] = [0.0, 0.00017200000000000001, 0.00034400000000000001, '...', 0.42346400000000001]
        @units = m
        @vector = [ 1.  0.  0.]
        @transformation_type = translation
        @depends_on = /entry/instrument/detector/transformations/rotation
      y:NX_FLOAT64[2527] = [0.0, 0.00017200000000000001, 0.00034400000000000001, '...', 0.43447200000000002]
        @units = m
        @vector = [ 0.  1.  0.]
        @transformation_type = translation
        @depends_on = /entry/instrument/detector/transformations/rotation
    instrument:NXinstrument
      @NX_class = NXinstrument
      attenuator:NXattenuator
        @NX_class = NXattenuator
        attenuator_transmission:NX_FLOAT64[] = 
      detector:NXdetector
        @NX_class = NXdetector
        beam_center_x:NX_FLOAT64[] = 
          @units = m
        beam_center_y:NX_FLOAT64[] = 
          @units = m
        count_time:NX_FLOAT64[450] = [0.049000000000000002, 0.049000000000000002, 0.049000000000000002, '...', 0.049000000000000002]
          @units = s
        data:NX_INT32[450,2527,2463] = __array
          __array = [
              [
                  [-2, 0, 0, '...', -2]
                  [0, 0, 0, '...', 0]
                  [0, 0, 0, '...', 0]
                  ...
                  [-2, -2, -2, '...', -2]
                ]
              [
                  [-2, 0, 0, '...', -2]
                  [0, 0, 0, '...', 0]
                  [0, 0, 0, '...', 0]
                  ...
                  [-2, -2, -2, '...', -2]
                ]
              [
                  [-2, 0, 0, '...', -2]
                  [0, 0, 0, '...', 0]
                  [0, 0, 0, '...', 0]
                  ...
                  [-2, -2, -2, '...', -2]
                ]
              ...
              [
                  [-2, 0, 0, '...', -2]
                  [0, 0, 0, '...', 0]
                  [0, 0, 0, '...', 0]
                  ...
                  [-2, -2, -2, '...', -2]
                ]
            ]
          @signal = 1
        dead_time:NX_FLOAT64[] = 
          @units = s
        depends_on:NX_CHAR[52] = /entry/instrument/detector/transformations/rotation
        description:NX_CHAR[25] = PILATUS3 6M, S/N 60-0126
        distance:NX_FLOAT64[450] = [0.26880999999999999, 0.26880999999999999, 0.26880999999999999, '...', 0.26880999999999999]
          @units = m
        frame_time:NX_FLOAT64[450] = [0.050000000000000003, 0.050000000000000003, 0.050000000000000003, '...', 0.050000000000000003]
          @units = s
        gain_setting:NX_CHAR[20] = autog (vrf = 1.000)
        saturation_value:NX_INT32[450] = [499798, 499798, 499798, '...', 499798]
        sensor_material:NX_CHAR[8] = Silicon
        sensor_thickness:NX_FLOAT64[] = 
          @units = m
        threshold_energy:NX_FLOAT64[] = 
          @units = eV
        type:NX_CHAR[12] = pixel array
        x_pixel_offset:NX_FLOAT64[2463] = [0.0, 0.00017200000000000001, 0.00034400000000000001, '...', 0.42346400000000001]
          @units = m
          @vector = [ 1.  0.  0.]
          @transformation_type = translation
          @depends_on = /entry/instrument/detector/transformations/rotation
        x_pixel_size:NX_FLOAT64[] = 
          @units = m
        y_pixel_offset:NX_FLOAT64[2527] = [0.0, 0.00017200000000000001, 0.00034400000000000001, '...', 0.43447200000000002]
          @units = m
          @vector = [ 0.  1.  0.]
          @transformation_type = translation
          @depends_on = /entry/instrument/detector/transformations/rotation
        y_pixel_size:NX_FLOAT64[] = 
          @units = m
        pilatus_diagnostics:NXcollection
          @NX_class = NXcollection
          Excluded_pixels:NX_CHAR[18] = badpixel_mask.tif
          Flat_field:NX_CHAR[40] = FF_p60-0126_E12700_T6350_vrf_m0p100.tif
          N_excluded_pixels:NX_CHAR[5] = 1137
          Trim_file:NX_CHAR[26] = p60-0126_E12700_T6350.bin
        transformations:NXtransformations
          @NX_class = NXtransformations
          rotation:NX_FLOAT64[450] = [180.0, 180.0, 180.0, '...', 180.0]
            @units = deg
            @transformation_type = rotation
            @depends_on = /entry/instrument/detector/transformations/translation
            @vector = [ 0.  0.  1.]
          translation:NX_FLOAT64[450] = [0.26880999999999999, 0.26880999999999999, 0.26880999999999999, '...', 0.26880999999999999]
            @units = m
            @offset_units = m
            @transformation_type = translation
            @depends_on = .
            @vector = [ 0.  0.  1.]
            @offset = [-0.21472652 -0.20996384  0.        ]
    sample:NXsample
      @NX_class = NXsample
      depends_on:NX_CHAR[45] = /entry/sample/transformations/CBF_axis_omega
      beam:NXbeam
        @NX_class = NXbeam
        incident_polarisation_stokes:NX_FLOAT64[450,4] = __array
          __array = [
              [1.0, 0.98999999999999999, 0.0, 0.0]
              [1.0, 0.98999999999999999, 0.0, 0.0]
              [1.0, 0.98999999999999999, 0.0, 0.0]
              ...
              [1.0, 0.98999999999999999, 0.0, 0.0]
            ]
        incident_wavelength:NX_FLOAT64[450] = [0.97624999999999995, 0.97624999999999995, 0.97624999999999995, '...', 0.97624999999999995]
          @units = angstroms
      transformations:NXtransformations
        @NX_class = NXtransformations
        CBF_axis_omega:NX_FLOAT64[450] = [0.0, 0.10000000000000001, 0.20000000000000001, '...', 44.899999999999999]
          @units = deg
          @transformation_type = rotation
          @depends_on = .
          @vector = [ 1.  0.  0.]

prjemian commented 9 years ago

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

yayahjb commented 9 years ago

We already use the same attribute names with different meanings depending on the group or field of which it is an attribute, vis. signal, but more importantly, now that we have adopted HDF5 there is no reason to expose the use of offset in its array index definitional meaning at all (vis the discussion about not exposing exactly this information in the mapping from CBF to NeXus). I propose that the current array offset index definitional meaning:

offset (_NXINT)Rank values of offsets to use for each dimension if the data is not in C storage order

be deprecated and that we say offset (_NXINT)Formerly: "Rank values of offsets to use for each dimension if the data is not in C storage order" no longer used in favor of HDF5 control of data storage and to avoid confusion with

offset (NX_NUMBER) A fixed offset applied before a transformation (three vector components).

I propose the array stride attribute also be deprecated for the same reason -- redundant relative to HDF5. -- Herbert

On Thu, Feb 12, 2015 at 5:34 PM, Pete R Jemian notifications@github.com wrote:

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

— Reply to this email directly or view it on GitHub https://github.com/nexusformat/definitions/issues/273#issuecomment-74168129 .

zjttoefs commented 9 years ago

I do agree with Herbert.

We need to be more careful in the future and actually check that we do not re-define existing things.

I haven't seen file with the "old" offset and that should be rare anyway. The geometry offset is the 99% case.

prjemian commented 9 years ago

Even if we agree to deprecate the offset: Rank values ... attribute, the problem still remains that two different definitions of the offset attribute remain. Both of these attributes available for use by any field.

One of the purposes of the NIAC is to avoid such name contention. The newest usage of offset must be the one renamed.

On 2/12/2015 5:27 PM, Herbert J. Bernstein wrote:

We already use the same attribute names with different meanings depending on the group or field of which it is an attribute, vis. signal, but more importantly, now that we have adopted HDF5 there is no reason to expose the use of offset in its array index definitional meaning at all (vis the discussion about not exposing exactly this information in the mapping from CBF to NeXus). I propose that the current array offset index definitional meaning:

offset (_NXINT)Rank values of offsets to use for each dimension if the data is not in C storage order

be deprecated and that we say offset (_NXINT)Formerly: "Rank values of offsets to use for each dimension if the data is not in C storage order" no longer used in favor of HDF5 control of data storage and to avoid confusion with

offset (NX_NUMBER) A fixed offset applied before a transformation (three vector components).

I propose the array stride attribute also be deprecated for the same reason -- redundant relative to HDF5. -- Herbert

On Thu, Feb 12, 2015 at 5:34 PM, Pete R Jemian notifications@github.com wrote:

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

— Reply to this email directly or view it on GitHub

https://github.com/nexusformat/definitions/issues/273#issuecomment-74168129 .

— Reply to this email directly or view it on GitHub https://github.com/nexusformat/definitions/issues/273#issuecomment-74175910.

yayahjb commented 9 years ago

Are we also going to rename the newer "signal" attribute? Are all attribute names global in scope? Are we saying the tree structure is not relevant to the use of particular attributes? Personally, I would prefer to stick with Tobias' original names, "offset" and "offset_units".

But, if a new name is needed, we could, say, use @base_offset=[...] and @base_offset_units="mm".

On Fri, Feb 13, 2015 at 8:21 AM, Pete R Jemian notifications@github.com wrote:

Even if we agree to deprecate the offset: Rank values ... attribute, the problem still remains that two different definitions of the offset attribute remain. Both of these attributes available for use by any field.

One of the purposes of the NIAC is to avoid such name contention. The newest usage of offset must be the one renamed.

On 2/12/2015 5:27 PM, Herbert J. Bernstein wrote:

We already use the same attribute names with different meanings depending on the group or field of which it is an attribute, vis. signal, but more importantly, now that we have adopted HDF5 there is no reason to expose the use of offset in its array index definitional meaning at all (vis the discussion about not exposing exactly this information in the mapping from CBF to NeXus). I propose that the current array offset index definitional meaning:

offset (_NXINT)Rank values of offsets to use for each dimension if the data is not in C storage order

be deprecated and that we say offset (_NXINT)Formerly: "Rank values of offsets to use for each dimension if the data is not in C storage order" no longer used in favor of HDF5 control of data storage and to avoid confusion with

offset (NX_NUMBER) A fixed offset applied before a transformation (three vector components).

I propose the array stride attribute also be deprecated for the same reason -- redundant relative to HDF5. -- Herbert

On Thu, Feb 12, 2015 at 5:34 PM, Pete R Jemian <notifications@github.com

wrote:

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

— Reply to this email directly or view it on GitHub

< https://github.com/nexusformat/definitions/issues/273#issuecomment-74168129

.

— Reply to this email directly or view it on GitHub < https://github.com/nexusformat/definitions/issues/273#issuecomment-74175910 .

— Reply to this email directly or view it on GitHub https://github.com/nexusformat/definitions/issues/273#issuecomment-74252283 .

mkoennecke commented 9 years ago

Hi all,

Am 13.02.2015 um 15:00 schrieb Herbert J. Bernstein notifications@github.com<mailto:notifications@github.com>:

Are we also going to rename the newer "signal" attribute? Are all attribute names global in scope? Are we saying the tree structure is not relevant to the use of particular attributes? Personally, I would prefer to stick with Tobias' original names, "offset" and "offset_units".

But, if a new name is needed, we could, say, use @base_offset=[...] and @base_offset_units="mm".

I think we were on this offset topic in Telcos in last year. My notes say that we decided to rename the offset belonging to the offset/stride pair to data_offset in the Telco in week 4 of 2014. I believed I even changed that in the manual but it seems as if I did not as it is not there.

Best Regards,

 Mark

On Fri, Feb 13, 2015 at 8:21 AM, Pete R Jemian notifications@github.com<mailto:notifications@github.com> wrote:

Even if we agree to deprecate the offset: Rank values ... attribute, the problem still remains that two different definitions of the offset attribute remain. Both of these attributes available for use by any field.

One of the purposes of the NIAC is to avoid such name contention. The newest usage of offset must be the one renamed.

On 2/12/2015 5:27 PM, Herbert J. Bernstein wrote:

We already use the same attribute names with different meanings depending on the group or field of which it is an attribute, vis. signal, but more importantly, now that we have adopted HDF5 there is no reason to expose the use of offset in its array index definitional meaning at all (vis the discussion about not exposing exactly this information in the mapping from CBF to NeXus). I propose that the current array offset index definitional meaning:

offset (_NXINT)Rank values of offsets to use for each dimension if the data is not in C storage order

be deprecated and that we say offset (_NXINT)Formerly: "Rank values of offsets to use for each dimension if the data is not in C storage order" no longer used in favor of HDF5 control of data storage and to avoid confusion with

offset (NX_NUMBER) A fixed offset applied before a transformation (three vector components).

I propose the array stride attribute also be deprecated for the same reason -- redundant relative to HDF5. -- Herbert

On Thu, Feb 12, 2015 at 5:34 PM, Pete R Jemian notifications@github.com<mailto:notifications@github.com

wrote:

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

— Reply to this email directly or view it on GitHub

< https://github.com/nexusformat/definitions/issues/273#issuecomment-74168129

.

— Reply to this email directly or view it on GitHub < https://github.com/nexusformat/definitions/issues/273#issuecomment-74175910 .

— Reply to this email directly or view it on GitHub https://github.com/nexusformat/definitions/issues/273#issuecomment-74252283 .

— Reply to this email directly or view it on GitHubhttps://github.com/nexusformat/definitions/issues/273#issuecomment-74257055.

prjemian commented 9 years ago

That's the answer then. Thanks. On Feb 13, 2015 8:11 AM, "mkoennecke" notifications@github.com wrote:

Hi all,

Am 13.02.2015 um 15:00 schrieb Herbert J. Bernstein < notifications@github.commailto:notifications@github.com>:

Are we also going to rename the newer "signal" attribute? Are all attribute names global in scope? Are we saying the tree structure is not relevant to the use of particular attributes? Personally, I would prefer to stick with Tobias' original names, "offset" and "offset_units".

But, if a new name is needed, we could, say, use @base_offset=[...] and @base_offset_units="mm".

I think we were on this offset topic in Telcos in last year. My notes say that we decided to rename the offset belonging to the offset/stride pair to data_offset in the Telco in week 4 of 2014. I believed I even changed that in the manual but it seems as if I did not as it is not there.

Best Regards,

Mark

On Fri, Feb 13, 2015 at 8:21 AM, Pete R Jemian <notifications@github.com mailto:notifications@github.com> wrote:

Even if we agree to deprecate the offset: Rank values ... attribute, the problem still remains that two different definitions of the offset attribute remain. Both of these attributes available for use by any field.

One of the purposes of the NIAC is to avoid such name contention. The newest usage of offset must be the one renamed.

On 2/12/2015 5:27 PM, Herbert J. Bernstein wrote:

We already use the same attribute names with different meanings depending on the group or field of which it is an attribute, vis. signal, but more importantly, now that we have adopted HDF5 there is no reason to expose the use of offset in its array index definitional meaning at all (vis the discussion about not exposing exactly this information in the mapping from CBF to NeXus). I propose that the current array offset index definitional meaning:

offset (_NXINT)Rank values of offsets to use for each dimension if the data is not in C storage order

be deprecated and that we say offset (_NXINT)Formerly: "Rank values of offsets to use for each dimension if the data is not in C storage order" no longer used in favor of HDF5 control of data storage and to avoid confusion with

offset (NX_NUMBER) A fixed offset applied before a transformation (three vector components).

I propose the array stride attribute also be deprecated for the same reason -- redundant relative to HDF5. -- Herbert

On Thu, Feb 12, 2015 at 5:34 PM, Pete R Jemian < notifications@github.commailto:notifications@github.com

wrote:

There is a conflict between the attribute "offset" as used here and the existing "offset" attribute for fields:

The stride and offset attributes are used together to index the array of data items in a multi-dimensional array.

Need to pick a new name here.

— Reply to this email directly or view it on GitHub

<

https://github.com/nexusformat/definitions/issues/273#issuecomment-74168129

.

— Reply to this email directly or view it on GitHub <

https://github.com/nexusformat/definitions/issues/273#issuecomment-74175910

.

— Reply to this email directly or view it on GitHub < https://github.com/nexusformat/definitions/issues/273#issuecomment-74252283>

.

— Reply to this email directly or view it on GitHub< https://github.com/nexusformat/definitions/issues/273#issuecomment-74257055>.

— Reply to this email directly or view it on GitHub https://github.com/nexusformat/definitions/issues/273#issuecomment-74258611 .

rayosborn commented 9 years ago

I don't have a particular view on this issue, but I am a little concerned about making changes during unminuted telcos. Shouldn't these decisions then be put to a vote of the NIAC, if only to make sure they are documented?

prjemian commented 9 years ago

see changeset ed22839

prjemian commented 9 years ago

The change from ed22839 was at line 829: "offset" changed to "data_offset" and a note added at the end of the attribute's documentation.

zjttoefs commented 9 years ago

You are correct, Ray.

Telcos now are minuted, that's the first half of the problem. In future if we have items that require a NIAC vote, I'd suggest creating a ticket with the "NIAC" label. Then we're relatively sure it ends up on the agenda.

Whether we want to vote now (online) or delay that until the next NIAC or not vote at all, I'm not sure about. I'd probably go with the middle, but send out for comments.

mkoennecke commented 9 years ago

Hi,

Am 13.02.2015 um 16:41 schrieb Tobias Richter notifications@github.com<mailto:notifications@github.com>:

You are correct, Ray.

Telcos now are minuted, that's the first half of the problem. In future if we have items that require a NIAC vote, I'd suggest creating a ticket with the "NIAC" label. Then we're relatively sure it ends up on the agenda.

Whether we want to vote now (online) or delay that until the next NIAC or not vote at all, I'm not sure about. I'd probably go with the middle, but send out for comments.

as I remember it, at the Telco we were of the opinion that this does not require a vote. It is a small technical fix to resolve a name conflict. Does not change anything to how NeXus works. But if you want a vote on this one, I suggest we do it by email. No need to defer this small problem to the next NIAC. I think it is not controversial either.

Regards,

Mark

— Reply to this email directly or view it on GitHubhttps://github.com/nexusformat/definitions/issues/273#issuecomment-74273041.

prjemian commented 9 years ago

Need more understanding of both "vector" and "offset" to make the documentation useful. The example in the GitHub issue is useful but only to a certain point. Not enough to provide clear understanding.

    y_pixel_offset:NX_FLOAT64[2527] = [0.0, 0.00017200000000000001, 0.00034400000000000001, '...', 0.43447200000000002]
      @units = m
      @vector = [ 0.  1.  0.]
      @transformation_type = translation
      @depends_on = /entry/instrument/detector/transformations/rotation

(Note: "depends_on", not "depends")

This points to the next transformation in the chain:

      rotation:NX_FLOAT64[450] = [180.0, 180.0, 180.0, '...', 180.0]
        @units = deg
        @transformation_type = rotation
        @depends_on = /entry/instrument/detector/transformations/translation
        @vector = [ 0.  0.  1.]

and the dependencies chain ends with:

      translation:NX_FLOAT64[450] = [0.26880999999999999, 0.26880999999999999, 0.26880999999999999, '...', 0.26880999999999999]
        @units = m
        @offset_units = m
        @transformation_type = translation
        @depends_on = .
        @vector = [ 0.  0.  1.]
        @offset = [-0.21472652 -0.20996384  0.        ]

Note the field y_pixel_offset:NX_FLOAT64[2527] and then the transformation fields rotation:NX_FLOAT64[450] and translation:NX_FLOAT64[450]? Why the different array lengths? How is this handled? Might help to show the math as well.

prjemian commented 9 years ago

see: http://download.nexusformat.org/doc/html/design.html#coordinate-transformations

prjemian commented 9 years ago

other places in the manual, check the index for depends_on (since search is not working now)

zjttoefs commented 9 years ago

assigned to me to add to the documentation

nexusformat / definitions

implement depends_on #273