NickAkhmetov/Log Normalization Edits #68
Conversation
…or clarity, move non-react component exports out to separate utils file
| @@ -0,0 +1,2 @@ | |||
| export const NORMALIZATIONS = ["None", "Row", "Column", "Log"] as const; | |||
There was a problem hiding this comment.
(I know I also treated it like a normalization but) in many ways I wonder if Log is really a normalization in the same way of row/column, or whether it's a different type of transformation. I can imagine many more transformations of the data, which are not normalizations. Perhaps at a certain point we want to allow users to add their own types of normalizations.
Although, perhaps row/col normalization is also a transformation.
What I am getting it, is that perhaps we want to handle these differently. Perhaps at a certain point, someone wants to do a transformation (like log) as well as a normalization (just row/col). Normalization of none/row/col is only possible on it's own (you can't normalize by both row and col), but in principle, many transformations are possible.
What do you think?
There was a problem hiding this comment.
That's a really good point; I think that a separate transformations context and util would be a good solution in the long run.
| const theme = heatmapThemes[heatmapTheme]; | ||
|
|
||
| const range = colorThresholds.map((_, idx) => idx / 255); | ||
| const domain = colorThresholds.map((_, idx) => idx / 255); |
There was a problem hiding this comment.
In the original:
- The
rangelist was generated fromcolorThresholds - Every scale mapped over the
rangeobject to create their own range/domain lists, duplicating this calculation - The
percentageScalehad a semantically awkwarddomain: rangeproperty assignment, which made things difficult to follow.
In the edited version:
- The
rangelist is renamed todomainso that it can be provided directly to thedomainproperty ofpercentageScale; the domain lists are still calculated the same way, but with a semantically accurate name for the source list. - A
rangelist that can be passed directly to all three scales without needing to be reinstantiated is created on the next line, which saves a bit of memory
Since these values are only used within the context of this useMemo, this should not cause any cascading changes.
This pull request introduces significant updates to the color scaling and normalization logic across multiple files, aiming to improve code clarity, modularity, and maintainability. Key changes include renaming the default
scalein theColorScaleContexttocountsScaleto make it easier to distinguish from the percentage and log scales, moving the normalization constant and type to a dedicated utility file to ensure that vite appropriately handles component refresh, and refactoring normalization-related logic in visualization components to avoid nested ternary statements.Updates to Color Scaling:
scaletocountsScaleinColorScaleContextto clarify its purpose and added consistent use ofdomainandrangefor better readability (src/contexts/ColorScaleContext.tsx, [1] [2].Refactoring Normalization Logic:
Normalizationtype andNORMALIZATIONSconstant fromNormalizationContextto a new utility file,src/utils/normalizations.ts, to centralize normalization-related definitions (src/contexts/NormalizationContext.tsx, [1];src/utils/normalizations.ts, [2].useIsNormalizedtouseIsNormalizedByRowOrColumnfor better semantic clarity following the introduction of log normalization (src/contexts/NormalizationContext.tsx, src/contexts/NormalizationContext.tsxL48-R45).Visualization Component Enhancements:
useLegendLabel,useMinValueLabel,useMaxValueLabel) inLegendto encapsulate label logic and improve readability (src/visx-visualization/Legend.tsx, [1] [2].useCurrentNormalizedScaleinHeatmapto abstract scale selection logic based on normalization type, replacing repetitive conditional checks (src/visx-visualization/heatmap/Heatmap.tsx, [1] [2] [3].Code Simplification and Error Handling:
DragOverlayusing aswitchstatement for improved readability and added a default case for error reporting. If we add more normalization options in the future, this will alert us that we need to add a case to handle it here as well. (src/visx-visualization/heatmap/DragOverlay.tsx, src/visx-visualization/heatmap/DragOverlay.tsxL263-R280).