chore(a11y): update color schemes for color-blind assessment #110#256
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor color palette management by centralizing hardcoded color dictionaries into a reusable Common enum in global_variables.py and introducing a dynamic colorscale builder function in global_scheme.py. Wind rose bins are extracted as a public constant and integrated throughout the template graphs module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce new centralized constants and a helper function requiring logic validation, but follow consistent patterns across multiple file updates. Most edits are repetitive palette references in variable definitions, reducing complexity. The new colorscale builder and bin constant integration are straightforward and localized. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tip For best results, initiate chat on the files or code changes. @FedericoTartarini: Hello! How can I help you with this pull request? I can:
Please let me know what you'd like me to do! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pages/lib/global_variables.py (1)
4-5: Verify that colorcet is in project dependencies.The new
colorcetlibrary is imported but may not be listed in the project's dependencies. Ensure it's added to requirements files to prevent runtime import errors.This is the same concern raised for
pages/lib/global_scheme.py. The verification script provided there will check both files.
🧹 Nitpick comments (1)
pages/lib/global_scheme.py (1)
20-20: Eliminate code duplication with global_variables.py.The exact formula for generating
wind_speed_coloris duplicated at line 274 ofpages/lib/global_variables.py. This violates the DRY principle and creates a maintenance burden.Consider extracting this into a shared utility function or referencing a single source of truth. For example, define it once in
global_variables.pyand import it here:In
global_scheme.py:+from pages.lib.global_variables import Variables + -wind_speed_color = [cc.CET_L19[int(round(i*(len(cc.CET_L19)-1)/(9)))] for i in range(10)] +wind_speed_color = Variables.WIND_SPEED.get_color()Or define it in a shared constants module if you prefer to keep the files independent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pages/lib/global_scheme.py(2 hunks)pages/lib/global_variables.py(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python Testing 🐍
pages/lib/global_scheme.py
[error] 1-1: Command 'pipenv run ruff format --check .' reported formatting issues; would reformat this file. Run 'ruff format' to fix code style issues.
pages/lib/global_variables.py
[error] 1-1: Command 'pipenv run ruff format --check .' reported formatting issues; would reformat this file. Run 'ruff format' to fix code style issues.
🔇 Additional comments (7)
pages/lib/global_scheme.py (2)
21-21: LGTM! Viridis is color-blind friendly.The switch to the Viridis color scheme for wind direction improves accessibility for color-blind users while maintaining good visual distinction.
22-26: cloud_colors is unused in the codebase; the review concern is invalid.The variable
cloud_colorsis defined inpages/lib/global_scheme.pybut is never imported or referenced anywhere else. Multiple files import various symbols fromglobal_scheme.py(such asmonth_lst,template,dropdown_names), but none importcloud_colors. Therefore, changing its length from 2 to 3 colors has no impact on any code that assumes a specific array length or uses hardcoded indices.Likely an incorrect or invalid review comment.
pages/lib/global_variables.py (5)
262-268: LGTM! Viridis is color-blind friendly.The switch to the Viridis color scheme for wind direction improves accessibility for color-blind users. The use of
list(pseq.Viridis)creates a mutable copy of the color sequence, which is appropriate for this configuration.
269-276: LGTM! CET_L19 is optimized for perceptual uniformity.The switch to Colorcet's CET_L19 palette improves color-blind accessibility. The formula correctly samples 10 evenly-spaced colors from the palette using
(len(cc.CET_L19)-1)/9to ensure the last index maps to the last color.Note: This formula is duplicated in
pages/lib/global_scheme.pyline 20. See the refactoring suggestion there.
278-288: No hardcoded assumptions for TOT_SKY_COVER color array length. Prepending #08306b is safe.
289-299: No issues identified; original review comment is incorrect.Verification shows all OPAQUE_SKY_COVER references in the codebase use only the
.col_nameattribute (global_scheme.py and extract_df.py). The color array is accessed generically viaget_color()method with no hardcoded length assumptions. Additionally, TOT_SKY_COVER already uses an identical 3-element color array, demonstrating this pattern is established and compatible throughout the codebase.
300-311: No impact on visualization compatibility—change is safe to proceed.The color array is accessed dynamically throughout the codebase:
var_color[0]andvar_color[-1]extract endpoints regardless of lengthvar_color[len(var_color) // 2]correctly identifies the middle element (2 colors → index 1; 3 colors → index 1)- All
colorscale=var_colorusages pass the array directly to Plotly, which handles variable-length gradients automaticallyNo hardcoded length assumptions or specific index dependencies exist. The addition of
#08306bto the VIS color array is fully compatible with all visualization code.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pages/lib/template_graphs.py (1)
501-505: Duplicate logic; consolidate with heatmap_with_filter.This wind speed bin handling duplicates the logic at lines 433-437. After fixing
WIND_ROSE_BINSand importing it, apply the same refactor here to eliminate duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pages/lib/global_scheme.py(2 hunks)pages/lib/template_graphs.py(2 hunks)requirements.txt(1 hunks)
🔇 Additional comments (3)
requirements.txt (1)
10-10: LGTM! Dependency added correctly.The colorcet dependency is properly added with an appropriate version constraint that allows minor and patch updates within the 3.x series.
pages/lib/global_scheme.py (2)
2-3: LGTM! Imports added for new color palette sources.The imports enable dynamic color generation from colorcet and plotly sequential palettes, which aligns with the PR's accessibility objectives.
39-48: LGTM! Color palette updates align with accessibility goals.The dynamic generation of
wind_speed_colorscale_rose, the switch to Viridis for wind direction, and the updated cloud colors with the new leading color#08306bsupport the PR's objective to improve color-blind accessibility.
|
@FedericoTartarini Please take a look when you have time. Thanks! |
…ove duplicate code
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pages/lib/global_scheme.py (1)
29-38: Add docstring and input validation.The
_stepped_colorscale_from_binsfunction lacks documentation and doesn't validate that the relationship between bins and colors is correct (should belen(bins) == len(colors) + 1).Based on past review comments, apply this diff to add documentation and validation:
def _stepped_colorscale_from_bins(bins, colors): + """Build a stepped colorscale from bin edges and colors. + + Args: + bins: List of N+1 bin edges defining N intervals + colors: List of N colors, one per interval + + Returns: + List of (position, color) tuples for Plotly colorscale + """ + if len(bins) != len(colors) + 1: + raise ValueError(f"Expected {len(colors)+1} bin edges for {len(colors)} colors, got {len(bins)}") vmin, vmax = bins[0], bins[-1]
🧹 Nitpick comments (2)
pages/lib/template_graphs.py (1)
566-566: Refactor to use centralized WIND_ROSE_BINS.The hardcoded bins at line 566 duplicate the
WIND_ROSE_BINSconstant defined inglobal_scheme.py. This creates a maintenance burden and risks inconsistency if bins are updated.Apply this diff to use the centralized constant:
- spd_bins = [-1, 0.5, 1.5, 3.3, 5.5, 7.9, 10.7, 13.8, 17.1, 20.7, np.inf] + spd_bins = list(WIND_ROSE_BINS) if si_ip == UnitSystem.IP: spd_bins = convert_bins(spd_bins)pages/lib/global_variables.py (1)
23-25: Code duplication: CET_L19_10 generation repeated.The same color generation logic appears in both
pages/lib/global_variables.py(lines 23-25) andpages/lib/global_scheme.py(lines 22-24). This duplication means any future changes must be made in two places.Consider one of these approaches:
Option 1 (recommended): Define
CET_L19_10inglobal_variables.pyonly and import it inglobal_scheme.py:In
pages/lib/global_scheme.py:+from pages.lib.global_variables import CET_L19_10 + -# Take 10 colors at equal intervals (including both ends) -wind_speed_color = [ - cc.CET_L19[int(round(i * (len(cc.CET_L19) - 1) / (9)))] for i in range(10) -] +wind_speed_color = CET_L19_10Option 2: Define it once in
global_scheme.pyand import inglobal_variables.py(but this would create a circular dependency risk sinceglobal_scheme.pyimports fromglobal_variables.py).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Pipfile(1 hunks)pages/lib/global_scheme.py(2 hunks)pages/lib/global_variables.py(12 hunks)pages/lib/template_graphs.py(3 hunks)
🔇 Additional comments (10)
Pipfile (1)
19-19: LGTM! Dependency properly added.The
colorcetdependency has been correctly added with appropriate version constraints, resolving the previous issue where the library was imported but not declared in dependencies.pages/lib/global_scheme.py (3)
2-4: LGTM! Required imports properly added.The new imports for
colorcet,numpy, andplotly.colors.sequentialare necessary for the color scheme improvements and are correctly positioned.
21-26: LGTM! Bin and color generation are correctly aligned.The color generation produces 10 colors from CET_L19, and WIND_ROSE_BINS defines 11 bin edges (10 intervals), which is the correct relationship for a stepped colorscale.
41-50: LGTM! Color schemes updated for accessibility.The wind speed colorscale, wind direction colors (Viridis), and cloud colors have been properly updated to improve color-blind accessibility as stated in the PR objectives.
pages/lib/template_graphs.py (3)
9-9: LGTM! Centralized bins imported.Importing
WIND_ROSE_BINSfromglobal_schemeestablishes a single source of truth for wind speed binning.
433-437: LGTM! Wind speed heatmap correctly uses centralized bins.The code properly:
- Uses
WIND_ROSE_BINSfrom the central definition- Converts bins for IP units when needed
- Sets
zmaxto the second-to-last bin (20.7), excludingnp.inffrom the color scale
501-505: LGTM! Consistent bin usage across heatmap functions.This heatmap function correctly mirrors the approach in
heatmap_with_filter, maintaining consistency.pages/lib/global_variables.py (3)
4-5: LGTM! Required imports added.The imports for
colorcetandplotly.colors.sequentialenable the centralized color palette generation.
7-31: LGTM! Centralized palettes improve maintainability.The centralized color palettes and unit conversion constants consolidate previously scattered color definitions, making them easier to maintain and update.
150-465: LGTM! Comprehensive color palette refactoring.The systematic replacement of hard-coded color arrays with centralized palettes across all
VariableInfoinstances improves consistency and maintainability. The use of conversion constants for IP ranges is also well-implemented.
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
FengW01
changed the title
refactored the code by moving these duplicated items to the beginning of the file. This allows other modules to directly reference them, reducing redundancy and improving maintainability. @FedericoTartarini Please review the update and let me know if any further adjustments are needed. Thank you! |
… Enum and drop colorcet in favor of hex color lists
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@FedericoTartarini thanks for your feedback! |
FedericoTartarini
merged commit fb9de71
into
CenterForTheBuiltEnvironment:development
2 participants
Update and unify color schemes to improve color-blind accessibility.
Summary by CodeRabbit