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

Merge attributes of key group and value dataset for CONTROL keys #498

Merged
merged 2 commits into from Mar 8, 2024

Conversation

philsmt
Copy link
Contributor

@philsmt philsmt commented Mar 7, 2024

Unfortunately I noticed that the HDF attributes mapping to Karabo properties at the time of recording are a bit all over the place in the case of CONTROL keys. For example, SA3_XTD10_XGM/XGM/DOOCS.beamposition.ixPos for a recent run:

├21 attributes:
│ ├accessMode: [2]
│ ├alias: 'IX.POS'
│ ├assignment: [0]
│ ├daqPolicy: [-1]
│ ├defaultValue: [-1.]
│ ├description: 'Calculated X position [mm]'
│ ├displayedName: 'X'
│ ├frac: [0]
│ ├leafType: [0]
│ ├metricPrefixEnum: [13]
│ ├metricPrefixName: 'milli'
│ ├metricPrefixSymbol: 'm'
│ ├nodeType: [0]
│ ├requiredAccessLevel: [0]
│ ├sec: [0]
│ ├tags: ['doocs' 'poll']
│ ├tid: [0]
│ ├unitEnum: [2]
│ ├unitName: 'meter'
│ ├unitSymbol: 'm'
│ └valueType: 'FLOAT'
├timestamp	[uint64: 500]
│ └4 attributes:
│   ├daqPolicy: [1]
│   ├frac: [0]
│   ├sec: [0]
│   └tid: [0]
└value	[float32: 500]
  └12 attributes:
    ├alias: 'IX.POS'
    ├daqPolicy: [1]
    ├frac: [0]
    ├metricPrefixEnum: [13]
    ├metricPrefixName: 'milli'
    ├metricPrefixSymbol: 'm'
    ├sec: [0]
    ├tags: ['doocs' 'poll']
    ├tid: [0]
    ├unitEnum: [2]
    ├unitName: 'meter'
    └unitSymbol: 'm'

After scanning through attributes across a large number of sources, I found:

  • In all cases except one (minSize, not shown here), the keys on value are a true subset of the parent
  • Duplicate attributes in value either have the same value (e.g. alias above) or a wrong, likely constant value (e.g. daqPolicy)
  • timestamp always has exactly the four attributes with exactly those values as shown above

Based on that, this PR merges the attributes on the parent group into those returned from KeyData.attributes() for the value dataset of a key, specifically overwriting any existing key.

@philsmt philsmt requested a review from takluyver March 7, 2024 12:22
@philsmt
Copy link
Contributor Author

philsmt commented Mar 7, 2024

Fixes #495


if self.is_control and self.key.endswith('.value'):
# For CONTROL sources, most of the attributes are saved on
# the parent group rather than the .value dataset.
Copy link
Member

Choose a reason for hiding this comment

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

Let's also mention here that at least currently, some overlapping attributes differ, and in that case the group level attribute seems to be the one that's right.

(I have a nasty foreboding that some day we'll come across a case where the group attribute is wrong and the dataset attribute is right, and we'll have to figure out how to wrangle that. But hopefully that's just pessimism. 🤞 )

Copy link
Member

Choose a reason for hiding this comment

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

(I have a nasty foreboding that some day we'll come across a case where the group attribute is wrong and the dataset attribute is right, and we'll have to figure out how to wrangle that. But hopefully that's just pessimism. 🤞 )

I don't think that's a possibility.
The attributes come from the device schema. The HDF5 datasets .value and .timestamp don't exist on the device. The parent group of those datasets, is actually (on the device) the property leaf that holds the value and attributes on the device we're saving.

So the attributes on these dataset must be copied from the parent group (or have some default). I guess the wrong default could as well be on the parent group, but I don't see how this could be wrong on the parent group and correct on the dataset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added that clarification.

In general though, I wouldn't worry much. I'll get in touch with the DAQ team and asked them to verify our assumptions here.

We may even be able to remove the dataset attributes entirely again and shift EXtra-data to the ones on the group. Frankly I doubt users would notice.

@takluyver
Copy link
Member

Other than that, LGTM. 👍

@takluyver takluyver added this to the 1.17 milestone Mar 8, 2024
@philsmt philsmt force-pushed the fix/merge-control-attributes branch from f057e96 to a736812 Compare March 8, 2024 15:03
@takluyver
Copy link
Member

Thanks, LGTM!

@philsmt
Copy link
Contributor Author

philsmt commented Mar 8, 2024

Thanks for review.

@philsmt philsmt merged commit e86750a into master Mar 8, 2024
10 checks passed
@takluyver takluyver deleted the fix/merge-control-attributes branch March 9, 2024 09:30
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.

Combine attributes on CONTROL key's parent group with .value attributes
3 participants