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

Fix support for enum/bool auxiliary signals in NX Line visualization #1738

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 21, 2025

assertNumericLikeNxData was asserting for NxData<NumericType> instead of NxData<NumericLikeType> ... but was correctly calling assertNumericLikeType internally! So MappedLineVis thought it could only receive numeric (nd)arrays (number[] | TypedArray) when in fact, at runtime, it could receive boolean[].

No errors were thrown at runtime because JS automatically coerces booleans to numbers in all mathematical operations (e.g. Math.min(false, 2) => 0). This typing bug would have been more problematic with bigint[], though.

I say "Fix support", because the fix still has invisible runtime implications: I now convert boolean auxiliary arrays to number arrays in MappedLineVis with a new hook called useToNumArrays, which avoids type coercion further down the line (e.g. in useDomains).

@axelboc axelboc requested a review from loichuder January 21, 2025 14:33
valueLabel?: string;
errors?: NumArray;
auxLabels?: string[];
auxValues?: NumArray[];
auxValues?: Value<Props['dataset']>[];
Copy link
Member

@loichuder loichuder Jan 22, 2025

Choose a reason for hiding this comment

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

I wonder if it might not be clearer to introduce a new type alias here to not reference dataset explicitely.

It seems weird since auxValues are not stored in dataset. It just so happens that auxiliary dataset values have the same type as the dataset values.

Copy link
Contributor Author

@axelboc axelboc Jan 23, 2025

Choose a reason for hiding this comment

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

Yeah, wasn't sure that it was clearer. Two goals here:

  1. The types of value and of each item in auxValues should match (if an NXdata group has a boolean signal, then it should be allowed to have boolean auxiliaries). This requires changing auxValues?: NumArray[] at least to ArrayValue<NumericLikeType> to match the value prop.
  2. The dtype passed to Dataset<ArrayShape, DType> should match the dtype passed to ArrayValue<Dtype>. In other words, I'd like to avoid the risk of the two becoming out of sync — e.g. Dataset<ArrayShape, NumericLikeType> and ArrayValue<NumericType>. This is why I refer to the type of the dataset prop with Value<Prop['dataset'].

Perhaps the second goal is not critical, since TypeScript would show an error in the container if the dtype of dataset and value/auxValues did not match. So should I stick with ArrayValue<NumericLikeType>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opening a PR to propose a normalisation of the mapped components' props.

Base automatically changed from distributive to main January 23, 2025 14:30
@axelboc axelboc merged commit e4dd359 into main Jan 23, 2025
8 checks passed
@axelboc axelboc deleted the fix-num-like-aux branch January 23, 2025 15: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.

2 participants