Skip to content

Conversation

@kbirken
Copy link
Member

@kbirken kbirken commented Jan 14, 2026

This PR extends some tests for physunits to reproduce the problem of infinite precision for types of prefixed units. It provides the fix, and updates the CHANGELOG accordingly.

This solves #1593.

@kbirken kbirken self-assigned this Jan 14, 2026
@kbirken kbirken force-pushed the bugfix/precision_of_types_with_prefixed_physunits_is_wrong_1593 branch from 3550500 to 316641f Compare January 14, 2026 22:00
@kbirken kbirken marked this pull request as ready for review January 14, 2026 22:19
@kbirken kbirken enabled auto-merge January 14, 2026 22:19
@kbirken kbirken requested a review from wsafonov January 14, 2026 22:19
Copy link
Collaborator

@wsafonov wsafonov left a comment

Choose a reason for hiding this comment

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

In general looks good, only couple of minor points:

  • naming: not sure if constant is the best way to describe the range with min == max bounds. When I see isConstant(), I think about a range that cannot be modified. Something like isSingular() or isSinglePoint() would be better imho.
  • in isContant(), checking both bounds are not equal NegInf / PosInf is covered by the last condition, since NegInf != PosInf. For safety, I would rather check min.isNotEmpty && max.isNotEmpty - even if this normally wouldn't happen, that definitely should not count as a single point range.

Not related to this bug, but I just observed that 2 kbyte + 300 bit gets accepted as number, since both of them are digital information units. But from the precision perspective that doesn't make sense. Not sure what would be the right approach here - compute everything as bits or even not allowing mix of byte/bits.

@kbirken kbirken disabled auto-merge January 15, 2026 10:24
@kbirken
Copy link
Member Author

kbirken commented Jan 15, 2026

In general looks good, only couple of minor points:

  • naming: not sure if constant is the best way to describe the range with min == max bounds. When I see isConstant(), I think about a range that cannot be modified. Something like isSingular() or isSinglePoint() would be better imho.
  • in isContant(), checking both bounds are not equal NegInf / PosInf is covered by the last condition, since NegInf != PosInf. For safety, I would rather check min.isNotEmpty && max.isNotEmpty - even if this normally wouldn't happen, that definitely should not count as a single point range.

Thanks, I just pushed a commit addressing these findings.

Not related to this bug, but I just observed that 2 kbyte + 300 bit gets accepted as number, since both of them are digital information units. But from the precision perspective that doesn't make sense. Not sure what would be the right approach here - compute everything as bits or even not allowing mix of byte/bits.

Which units did you use here? There are three ones in the standard library.

image

Guessing from the "k" prefix that you used metric scaling, there are implicit conversion rules which just multiply the number of bytes times 8 to retrieve bits and vice versa. But it seems this doesn't work, currently a result of 2300 byte is being computed (for binary scaling, the result is 2348).

image

Assuming the conversion rule is valid, IMO it should give the following results

  • metric scaling: 2037,5 byte or 16300 bit
  • binary scaling: 2085,5 byte or 16684 bit

So the addition of mixed units of the same quantity seems to be wrong for physunits, this is likely a topic for additional tickets....

@wsafonov
Copy link
Collaborator

Which units did you use here? There are three ones in the standard library.

Hmm, in my case there was only one bit unit available, namely the metric one, which is the correct one since we are expecting a metric-scaled value.

image

Does it behave differently on your side at the same location?

Copy link
Collaborator

@wsafonov wsafonov left a comment

Choose a reason for hiding this comment

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

Important points are all addressed, looks good now.

@kbirken
Copy link
Member Author

kbirken commented Jan 15, 2026

Does it behave differently on your side at the same location?

Yes, at this location the chunk only imports UnitsOfInformationMetric, so the binary scaled units are not available. But the computation with kbyte+bit is wrong anyway.

@kbirken
Copy link
Member Author

kbirken commented Jan 15, 2026

Important points are all addressed, looks good now.

Will squash and then merge.

Details:
- add failing tests showing the problem with infinite precisions
- compute proper precision for number types with prefixed unit
- update CHANGELOG
@kbirken kbirken force-pushed the bugfix/precision_of_types_with_prefixed_physunits_is_wrong_1593 branch from 9415804 to c2fd0ac Compare January 15, 2026 11:27
@kbirken kbirken enabled auto-merge January 15, 2026 11:27
@kbirken kbirken merged commit 909f7b2 into maintenance/mps20241 Jan 15, 2026
2 checks passed
@kbirken kbirken deleted the bugfix/precision_of_types_with_prefixed_physunits_is_wrong_1593 branch January 15, 2026 11:47
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