Conversation
ecb75a0 to
6a971a3
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded a helper to sample discrete colors from Plotly colorscales and extended Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xarray_plotly/common.py`:
- Around line 275-278: Update the docstring for the discrete_only parameter to
remove "bar" from the list of chart types that sample continuous colors into a
discrete sequence; specifically, edit the text near the discrete_only
description in xarray_plotly/common.py so it only lists line(), area(), box(),
and pie() (since bar() calls resolve_colors(colors, px_kwargs) without
discrete_only=True). Leave the rest of the description unchanged.
- Around line 254-258: The _sample_colorscale function is sampling a fixed n
(default 10) which causes color clustering for small categorical dims; change
resolve_colors so sampling uses the actual category count: either call
resolve_colors/_sample_colorscale after assign_slots so you can read the
cardinality from darray.sizes[color_dim], or refactor resolve_colors to accept
the resolved darray and the color dimension name and compute n =
int(darray.sizes[color_dim]) before calling _sample_colorscale; update all call
sites (including the calls that currently pass only colors when
discrete_only=True) to provide the proper n or the darray+color dim so samples
span the full colorscale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f275bb3a-1782-4698-9ac8-ed1ee7b15780
📒 Files selected for processing (2)
xarray_plotly/common.pyxarray_plotly/plotting.py
f5b74b8 to
40f2a6d
Compare
Passing a continuous colorscale like `colors='turbo'` to area(), line(), fast_bar(), box(), or pie() previously failed because `resolve_colors` set `color_continuous_scale` which these px functions don't accept. Now samples from the continuous scale into a discrete sequence for these chart types. bar() and scatter() natively support continuous scales so are left unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
40f2a6d to
23edbc3
Compare
Summary
colors='turbo') toarea(),line(),fast_bar(),box(), orpie()previously failed becauseresolve_colorssetcolor_continuous_scale, which these Plotly Express functions don't accept.discrete_onlyparameter toresolve_colors()that samples from the continuous scale into acolor_discrete_sequenceinstead.bar(),scatter(), andimshow()are unchanged (they natively supportcolor_continuous_scale).Test plan
colors='turbo'works on all 8 chart types🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements