Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several issues related to netCDF data handling, including chunk data distribution, rescaling logic, colorbar scaling, and date handling. The changes improve the accuracy and efficiency of data processing and rendering. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes, including a correction to chunk handling in NetCDF data processing, improved data rescaling logic, and a fix for date parsing. The changes appear to correctly address the issues described.
My review includes a few suggestions:
- In
Colorbar.tsx, I've recommended a minor refactoring to improve code consistency and readability when applying the scaling factor. - I've pointed out a potential inconsistency in the data rescaling logic between
NCGetters.tsandZarrGetters.ts. Aligning this logic would ensure consistent behavior across different data sources.
Overall, these are valuable improvements to the application's data handling capabilities.
| if (newScalingFactor != null | ||
| && newScalingFactor != scalingFactor){ // If the scalingFactor has changed, need to rescale main array. Not worried about shrinking values at the moment. | ||
| const thisScaling = scalingFactor ? newScalingFactor - scalingFactor : newScalingFactor | ||
| RescaleArray(typedArray, thisScaling) | ||
| scalingFactor = newScalingFactor | ||
| for (const id of rescaleIDs){ // Set new scalingFactor on the chunks | ||
| const tempName = `${cacheBase}_chunk_${id}` | ||
| const tempChunk = cache.get(tempName) | ||
| tempChunk.scaling = scalingFactor | ||
| RescaleArray(tempChunk.data, thisScaling) | ||
| cache.set(tempName, tempChunk) | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for rescaling has been changed to execute whenever newScalingFactor is different from scalingFactor. However, the corresponding logic in src/components/zarr/ZarrGetters.ts (lines 310-323) still retains the condition if (scalingFactor == null || newScalingFactor > scalingFactor). This inconsistency could lead to different scaling behaviors for NetCDF and Zarr sources. To ensure consistent behavior, the scaling logic should be unified for both data source types. Was this change intended to be applied to Zarr sources as well?
1 participant
Thankfully this was an easy solve but a difficult debug situation.
Unfortunately
Chunk2Array()was completely vibe coded. I didn't have the mental fortitude to decipher how to distribute chunk data into the main array. So while I understand what the function is doing I wasn't intimately familiar with the inputs. I thought thechunkShapeinput was to be the shape of the current chunk being sent to the array. Edge chunks are often smaller than the definedchunkshape.Actually the function just wanted the globally defined
chunkshape. And I never noticed that this was the value I was saving to cache. Which is why when I loaded the cached data it was being placed correctly.Closes #594
I also had some weird issues with small values and rescaling. I decided to forego additional rescaling if the next chunk is smaller than the previous AFTER scaling. Since the initial scaling would stretch the variance out, additional scaling is probably unnecessary.
I also fixed that the colorbar didn't apply
scalingFactoron first render.Lastly, I noticed the dates for the slices were off by a month. After some debugging and reading, I learned that the Month field in the javascript Datetime constructor is zero-indexed. Whereas Day and Year are not. Odd.