Skip to content

Add mock data and snapshot tests for _FillValue#1750

Merged
axelboc merged 2 commits intomainfrom
fill-value-snapshots
Feb 3, 2025
Merged

Add mock data and snapshot tests for _FillValue#1750
axelboc merged 2 commits intomainfrom
fill-value-snapshots

Conversation

@axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 30, 2025

  • Add a couple of mock datasets to test and demonstrate the use of the _FillValue attribute (and to double check that the fill value is used in the domain computation as expected).
_FillValue=400 _FillValue=400
image image
  • Add snapshot tests for those two datasets to prevent future regressions of the ignoreValue feature in the Line and Heatmap visualizations.
  • Introduce a type alias for the ignoreValue function.
  • Tweak the implementation of useIgnoreFillValue slightly to memoise a few more computations.
  • Fix the dynamic domain computation in some stories by passing ignoreValue (and errors for that matter) to useDomain.

@axelboc axelboc force-pushed the fill-value-snapshots branch from 1a2e23e to b187870 Compare January 30, 2025 15:56
...getDomains(
auxArrays,
scaleType,
showErrors ? auxErrorsArrays : undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't pass ignoreValue here, as the expected behaviour for auxiliary signals is not clear. Something to discuss in the future if the user need arises.

@axelboc axelboc requested a review from loichuder January 30, 2025 15:58
@axelboc
Copy link
Contributor Author

axelboc commented Jan 30, 2025

/approve

const isIgnored = ignoreValue ? ignoreValue(value) : false;
const bufferIndex = index * 2;

if (isIgnored) {
Copy link
Member

Choose a reason for hiding this comment

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

I liked the intermediate variable: brain computing .? can be tricky sometime 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I personally found the ternary more difficult to read. 😅 With ?., the fact that ignoreValue is optional doesn't distract as much from the main condition, which is whether or not the value is to be ignored.

@axelboc axelboc force-pushed the fill-value-snapshots branch from f00d7a0 to 19a39bb Compare February 3, 2025 09:59
@axelboc axelboc merged commit 02f6f73 into main Feb 3, 2025
8 checks passed
@axelboc axelboc deleted the fill-value-snapshots branch February 3, 2025 10:03
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