Conversation
loichuder
force-pushed
the
scaling-axes
branch
from
68985c5 to
8ff7a24
Compare
| value.forEach((v) => { | ||
| scaledValue.push(scalingFactor * Number(v) + offset); | ||
| }); | ||
| return scaledValue; |
There was a problem hiding this comment.
I tried to use value.map but to no avail: I got errors saying that BigInt could not be converted to number.
I think it has to do with the savage conversions I am doing in this part but I am not versed in TS enough to do better 😓
There was a problem hiding this comment.
Might be best to skip bigint arrays, since it will not work anyway? I'll push something.
|
|
||
| const offsetDataset = findOffset(group, datasetName); | ||
| const offset = offsetDataset | ||
| ? Number(valuesStore.get({ dataset: offsetDataset })) |
There was a problem hiding this comment.
The cast as Number could probably be replaced by an assert instead ?
There was a problem hiding this comment.
Yes, a couple of assertions would be best I think.
axelboc
left a comment
axelboc
left a comment
There was a problem hiding this comment.
According to the NXdata spec, these are a bit like errors: they are stored as datasets in the NXdata group with the name AXISNAME_scaling_factor and AXISNAME_offset.
So I had to work a bit around this design choice by allowing my getScalingInfo function to get data from valuesStore. Not sure if I should have made a hook out of it?
Argh NeXus!! 😂
The way we've dealt with this so far (e.g. with the title dataset) is to store the dataset objects in the NxData structure, and then fetch their values in NxValuesFetcher. I don't think there's a huge gain in doing so; the only ones I see are 1) separation of concerns and 2) value requests parallelisation in NxValuesFetcher (which would be more challenging to achieve in useNxData()).
I'll push a commit now for the bigint stuff, as mentioned in my first comment (since it requires moving a couple of type guards from somewhere else).
| value.forEach((v) => { | ||
| scaledValue.push(scalingFactor * Number(v) + offset); | ||
| }); | ||
| return scaledValue; |
There was a problem hiding this comment.
Might be best to skip bigint arrays, since it will not work anyway? I'll push something.
|
|
||
| const offsetDataset = findOffset(group, datasetName); | ||
| const offset = offsetDataset | ||
| ? Number(valuesStore.get({ dataset: offsetDataset })) |
There was a problem hiding this comment.
Yes, a couple of assertions would be best I think.
| return dataset; | ||
| } | ||
|
|
||
| export function findScalingFactor( |
There was a problem hiding this comment.
findScalingFactorDataset and findOffsetDataset
| dataset && { | ||
| dataset, | ||
| ...getDatasetInfo(dataset, attrValuesStore), | ||
| ...getScalingInfo(group, dataset.name, valuesStore), | ||
| }, |
There was a problem hiding this comment.
Maybe time to make a getAxisDef function instead of adding getScalingInfo?
| }; | ||
|
|
||
| export type AxisDef = DatasetDef<NumericType>; | ||
| export type AxisDef = DatasetDef<NumericType> & ScalingInfo; |
There was a problem hiding this comment.
How about:
interface AxisDef extends DatasetDef<NumericType> {
scalingFactor: number | undefined;
offset: number | undefined;
}There was a problem hiding this comment.
Yep, makes sense if we replace getScalingInfo by getAxisDef that will return AxisDef
|
I also meant to say: good thinking putting the scaling computation in |
| return value; | ||
| } | ||
|
|
||
| return value.map((v) => scalingFactor * v + offset); |
There was a problem hiding this comment.
Oh I'm just realising that the NeXus spec says this should be: 🤔
corrected values = (FIELDNAME + offset) * scaling_factor`
There was a problem hiding this comment.
Oopsie-daisy ! Thanks for checking 😹
|
@loichuder : Only feedback from me is make sure to document it so claude/ChatGPT/Gemini realize this is a useful option! |
3 participants
For #1943
POC for the support of
scaling_factorandoffset. According to the NXdata spec, these are a bit likeerrors: they are stored as datasets in theNXdatagroup with the nameAXISNAME_scaling_factorandAXISNAME_offset.So I had to work a bit around this design choice by allowing my
getScalingInfofunction to get data fromvaluesStore. Not sure if I should have made a hook out of it?Also, scaling could have unintended side effects since the scaled value may have a different type than the value store in the dataset itself (ex: with a scaling factor of
0.1, integers get converted to floats). I also have to do a bit of dirty conversions inNxValuesFetcherto work around this.Tell me what you think.