Skip to content

Fixing issue in CDB considering the channel id an int#158

Merged
pscheidler merged 3 commits intodevelopfrom
bugfix/channel_id_uint
May 21, 2025
Merged

Fixing issue in CDB considering the channel id an int#158
pscheidler merged 3 commits intodevelopfrom
bugfix/channel_id_uint

Conversation

@pscheidler
Copy link
Member

@pscheidler pscheidler commented May 16, 2025

The main issue is that the ChannelID element in the CDB is considered an Int, and most other places it is considered a UInt.

Small question: SubChannelsIDs are considered Ints, which is consistent, but odd. Should IDs generally just be uints?

@pscheidler pscheidler self-assigned this May 16, 2025
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.32%. Comparing base (f406a63) to head (c55276a).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #158   +/-   ##
========================================
  Coverage    75.32%   75.32%           
========================================
  Files           13       13           
  Lines         4016     4016           
========================================
  Hits          3025     3025           
  Misses         991      991           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@StokesMIDE
Copy link
Member

Small question: SubChannelsIDs are considered Ints, which is consistent, but odd. Should IDs generally just be uints?

All IDs (channels, subchannels, calibration, etc.) should probably be uints.
Similarly (but a different issue), many of the StringElements should be UnicodeElements. I think most were made StringElements (which are limited to printable 7-bit ASCII) as an oversight.

Copy link
Member

@StokesMIDE StokesMIDE left a comment

Choose a reason for hiding this comment

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

We should change all the ID/IDRef elements to UInteger. Since we don't use any higher than the low 100s, this shouldn't cause any problem with existing data. We might as well do it in one PR.

@pscheidler
Copy link
Member Author

Small question: SubChannelsIDs are considered Ints, which is consistent, but odd. Should IDs generally just be uints?

All IDs (channels, subchannels, calibration, etc.) should probably be uints. Similarly (but a different issue), many of the StringElements should be UnicodeElements. I think most were made StringElements (which are limited to printable 7-bit ASCII) as an oversight.

I'll leave this one for a following PR

<FloatElement name="BivariateCalReferenceValue" id="0x4B05" multiple="0" minver="1">Reference value for the 2nd channel when using bivariate calibration</FloatElement>
<UIntegerElement name="BivariateChannelIDRef" id="0x4B06" multiple="0" minver="1">Channel ID of the channel to be used as the 2nd parameter for bivariate compensation</UIntegerElement>
<UIntegerElement name="BivariateSubChannelIDRef" id="0x4B07" multiple="0" minver="1">SubChannel ID of the subchannel to be used as the 2nd parameter for bivariate compensation</UIntegerElement>
<IntegerElement name="BivariateSubChannelIDRef" id="0x4B07" multiple="0" minver="1">SubChannel ID of the subchannel to be used as the 2nd parameter for bivariate compensation</IntegerElement>
Copy link
Member

Choose a reason for hiding this comment

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

Was this changed the other way (UIntegerElement -> IntegerElement) on purpose? Do we ever want negative channel ID references here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'd changed it earlier when I thought the intent was for SubChannels to be ints

Suggested change
<IntegerElement name="BivariateSubChannelIDRef" id="0x4B07" multiple="0" minver="1">SubChannel ID of the subchannel to be used as the 2nd parameter for bivariate compensation</IntegerElement>
<UIntegerElement name="BivariateSubChannelIDRef" id="0x4B07" multiple="0" minver="1">SubChannel ID of the subchannel to be used as the 2nd parameter for bivariate compensation</UIntegerElement>

@pscheidler pscheidler merged commit a4254b1 into develop May 21, 2025
31 checks passed
@pscheidler pscheidler deleted the bugfix/channel_id_uint branch May 21, 2025 20:57
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.

3 participants