Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the worker-based array fetching logic, specifically deleting the GetCurrentArrayWorkers function and its associated worker file, while simplifying state management in the FlatMap component. Key feedback includes a logic regression in GetArray.ts where a check for local stores was removed, potentially affecting remote data access. Additionally, a performance regression was noted in TextureMakers.tsx due to moving a division operation inside a loop, and inconsistent indentation was identified in the HandleTimeSeries function.
| const { cache } = useCacheStore.getState(); | ||
| const useNC = initStore.startsWith("local") && fetchNC // In case a user has NetCDF switched but then goes to a remote | ||
| const fetcher = useNC ? NCFetcher() : zarrFetcher() | ||
| const fetcher = fetchNC ? NCFetcher() : zarrFetcher() |
There was a problem hiding this comment.
The removal of the initStore.startsWith("local") check is a logic regression. This condition ensured that NCFetcher was only used for local stores, providing a fallback to zarrFetcher for remote URLs even if the NetCDF toggle was active. Reverting this may cause errors when users attempt to access remote data while the NetCDF setting is enabled.
| const fetcher = fetchNC ? NCFetcher() : zarrFetcher() | |
| const useNC = initStore.startsWith("local") && fetchNC; | |
| const fetcher = useNC ? NCFetcher() : zarrFetcher(); |
| const multiplier = 1/range | ||
| for (let i = 0; i < data.length; i++){ | ||
| const normed = (data[i] - minVal) * multiplier; | ||
| const normed = (data[i] - minVal) / range; |
There was a problem hiding this comment.
This change introduces a performance regression by moving the division operation inside the loop. In the previous version, a reciprocal multiplier (1 / range) was calculated once outside the loop, allowing for more efficient multiplication within the loop. For large data arrays, this can have a noticeable impact on performance.
| const uv = event.uv; | ||
| const normal = new THREE.Vector3(0,0,1) | ||
| if(uv){ | ||
| const tsUV = flipY ? new THREE.Vector2(uv.x, 1-uv.y) : uv | ||
| const tempTS = GetTimeSeries({data:analysisMode ? analysisArray : GetCurrentArray(), shape:dataShape, stride:strides},{uv:tsUV,normal}) | ||
| setPlotDim(0) //I think this 2 is only if there are 3-dims. Need to rework the logic | ||
|
|
||
| const coordUV = parseUVCoords({normal:normal,uv:uv}) | ||
| let dimCoords = coordUV.map((val,idx)=>val ? dimSlices[idx][Math.round(val*dimSlices[idx].length)] : null) | ||
| const thisDimNames = dimNames.filter((_,idx)=> dimCoords[idx] !== null) | ||
| const thisDimUnits = dimUnits.filter((_,idx)=> dimCoords[idx] !== null) | ||
| dimCoords = dimCoords.filter(val => val !== null) | ||
| const tsID = `${dimCoords[0]}_${dimCoords[1]}` | ||
| const tsObj = { | ||
| color:evaluate_cmap(getColorIdx()/10,"Paired"), | ||
| data:tempTS | ||
| } | ||
| incrementColorIdx(); | ||
| updateTimeSeries({ [tsID] : tsObj}) | ||
| const dimObj = { | ||
| first:{ | ||
| name:thisDimNames[0], | ||
| loc:dimCoords[0] ?? 0, | ||
| units:thisDimUnits[0] | ||
| }, | ||
| second:{ | ||
| name:thisDimNames[1], | ||
| loc:dimCoords[1] ?? 0, | ||
| units:thisDimUnits[1] | ||
| }, | ||
| plot:{ | ||
| units:dimUnits[0] | ||
| } | ||
| } | ||
| updateDimCoords({[tsID] : dimObj}) | ||
| } | ||
| } |
There was a problem hiding this comment.
The indentation of the HandleTimeSeries function body is inconsistent with the rest of the file, using an excessive 8-space offset. This should be corrected to match the project's standard indentation (likely 2 spaces relative to the function declaration) to maintain readability.
const uv = event.uv;
const normal = new THREE.Vector3(0, 0, 1);
if (uv) {
const tsUV = flipY ? new THREE.Vector2(uv.x, 1 - uv.y) : uv;
const tempTS = GetTimeSeries({ data: analysisMode ? analysisArray : GetCurrentArray(), shape: dataShape, stride: strides }, { uv: tsUV, normal });
setPlotDim(0);
const coordUV = parseUVCoords({ normal: normal, uv: uv });
let dimCoords = coordUV.map((val, idx) => val ? dimSlices[idx][Math.round(val * dimSlices[idx].length)] : null);
const thisDimNames = dimNames.filter((_, idx) => dimCoords[idx] !== null);
const thisDimUnits = dimUnits.filter((_, idx) => dimCoords[idx] !== null);
dimCoords = dimCoords.filter(val => val !== null);
const tsID = `${dimCoords[0]}_${dimCoords[1]}`;
const tsObj = {
color: evaluate_cmap(getColorIdx() / 10, "Paired"),
data: tempTS
};
incrementColorIdx();
updateTimeSeries({ [tsID]: tsObj });
const dimObj = {
first: {
name: thisDimNames[0],
loc: dimCoords[0] ?? 0,
units: thisDimUnits[0]
},
second: {
name: thisDimNames[1],
loc: dimCoords[1] ?? 0,
units: thisDimUnits[1]
},
plot: {
units: dimUnits[0]
}
};
updateDimCoords({ [tsID]: dimObj });
}
}
Reverts #629
SHaredArrayBuffer is not recognized in prod