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

implement depends_on #273

Closed
stuartcampbell opened this issue May 5, 2014 · 22 comments
Closed

implement depends_on #273

stuartcampbell opened this issue May 5, 2014 · 22 comments
Assignees

Comments

@stuartcampbell
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

Original reporter: zjttoefs

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

@stuartcampbell
Copy link
Member Author

Original reporter: zjttoefs

Here's one:

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

@prjemian
Copy link
Contributor

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

@prjemian
Copy link
Contributor

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
Copy link
Contributor

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.

prjemian added a commit that referenced this issue Feb 12, 2015
* conflict between the attribute "offset" as used here and the existing "offset" attribute for fields - need to pick a new name
* need better documentation for each of the attributes in  <xs:attributeGroup name="dependenciesAttributes">
@yayahjb
Copy link
Contributor

yayahjb commented Feb 12, 2015

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 (NX_INT)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 (NX_INT)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
#273 (comment)
.

@zjttoefs
Copy link
Contributor

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
Copy link
Contributor

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 (NX_INT)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 (NX_INT)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

#273 (comment)
.


Reply to this email directly or view it on GitHub
#273 (comment).

@yayahjb
Copy link
Contributor

yayahjb commented Feb 13, 2015

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 (NX_INT)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 (NX_INT)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

<
#273 (comment)

.


Reply to this email directly or view it on GitHub
<
#273 (comment)
.


Reply to this email directly or view it on GitHub
#273 (comment)
.

@mkoennecke
Copy link
Contributor

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.commailto: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 (NX_INT)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 (NX_INT)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

<
#273 (comment)

.


Reply to this email directly or view it on GitHub
<
#273 (comment)
.


Reply to this email directly or view it on GitHub
#273 (comment)
.


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

@prjemian
Copy link
Contributor

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 (NX_INT)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 (NX_INT)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

<

#273 (comment)

.


Reply to this email directly or view it on GitHub
<

#273 (comment)

.


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
#273 (comment)
.

@rayosborn
Copy link
Contributor

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
Copy link
Contributor

see changeset ed22839

@prjemian
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Hi,

Am 13.02.2015 um 16:41 schrieb Tobias Richter <notifications@github.commailto: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//issues/273#issuecomment-74273041.

@prjemian
Copy link
Contributor

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 added a commit that referenced this issue Feb 14, 2015
@prjemian
Copy link
Contributor

@prjemian
Copy link
Contributor

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

@zjttoefs zjttoefs self-assigned this Feb 18, 2015
@zjttoefs
Copy link
Contributor

assigned to me to add to the documentation

prjemian added a commit that referenced this issue Feb 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants