Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new example notebook demonstrating end-to-end dataset plotting with xarray_plotly and introduces a new configuration option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @docs/examples/datasets.ipynb:
- Around line 112-116: The current note incorrectly states "Dimensions are:
(time, variable, city)" while ds.to_array().dims actually returns
('variable','time','city') because to_array() puts the new variable dimension
first; update the explanatory text (or remove the ds.to_array() print) to
clarify that the printed dims reflect to_array() behavior and that the dimension
reordering that moves "variable" to position 1 happens only when plotting via
the xpx(...).line accessor; reference ds.to_array() for the print and
xpx(ds).line for the plotting behavior so readers understand when each reorder
occurs.
In @xarray_plotly/accessor.py:
- Around line 363-371: The negative index handling for dataset_variable_position
is incorrect; replace the current "any negative => append" logic by computing a
proper insertion index using Python-style negative indexing: after building dims
= list(da.dims) and removing "variable", compute insert_idx = pos if pos >= 0
else len(dims) + pos + 1, then validate that 0 <= insert_idx <= len(dims) (raise
ValueError for out-of-range pos), and finally do dims.insert(insert_idx,
"variable") before da = da.transpose(*dims); reference symbols: da, dims, pos,
dataset_variable_position.
🧹 Nitpick comments (2)
xarray_plotly/config.py (1)
114-126: Consider adding input validation for dataset_variable_position.While the implementation correctly propagates the configuration value, there's no validation to ensure the position is within reasonable bounds. Consider adding validation to provide clear error messages if users provide invalid values.
♻️ Optional validation enhancement
@contextmanager def set_options( *, label_use_long_name: bool | None = None, label_use_standard_name: bool | None = None, label_include_units: bool | None = None, label_unit_format: str | None = None, slot_orders: dict[str, tuple[str, ...]] | None = None, dataset_variable_position: int | None = None, ) -> Generator[None, None, None]: """Set xarray_plotly options globally or as a context manager. Args: label_use_long_name: Use `long_name` attribute for labels. label_use_standard_name: Fall back to `standard_name` if `long_name` not available. label_include_units: Append units to labels. label_unit_format: Format string for units. Use `{units}` as placeholder. slot_orders: Slot orders per plot type. dataset_variable_position: Position of "variable" dim when plotting all Dataset variables. Default 1 (second, typically color). Use 0 for first, -1 for last. Yields: None when used as a context manager. + + Raises: + ValueError: If dataset_variable_position is invalid. Example: ```python from xarray_plotly import config, xpx # Use as context manager with config.set_options(label_include_units=False): fig = xpx(da).line() # No units in labels # Units are back after the context ``` """ # Store old values old_values = { "label_use_long_name": _options.label_use_long_name, "label_use_standard_name": _options.label_use_standard_name, "label_include_units": _options.label_include_units, "label_unit_format": _options.label_unit_format, "slot_orders": dict(_options.slot_orders), "dataset_variable_position": _options.dataset_variable_position, } + # Validate dataset_variable_position + if dataset_variable_position is not None: + if not isinstance(dataset_variable_position, int): + raise TypeError(f"dataset_variable_position must be an int, got {type(dataset_variable_position)}") # Apply new values (modify in place to keep reference) if label_use_long_name is not None: _options.label_use_long_name = label_use_long_name if label_use_standard_name is not None: _options.label_use_standard_name = label_use_standard_name if label_include_units is not None: _options.label_include_units = label_include_units if label_unit_format is not None: _options.label_unit_format = label_unit_format if slot_orders is not None: _options.slot_orders = dict(slot_orders) if dataset_variable_position is not None: _options.dataset_variable_position = dataset_variable_positionxarray_plotly/accessor.py (1)
358-358: Consider moving import to module level.The import of
_optionsis inside the method, which incurs a small overhead on every call. Since this is internal API usage within the same package, consider importing at the module level.♻️ Refactor: Move to module-level import
"""Accessor classes for Plotly Express plotting on DataArray and Dataset.""" from typing import Any, ClassVar import plotly.graph_objects as go from xarray import DataArray, Dataset from xarray_plotly import plotting from xarray_plotly.common import SlotValue, auto +from xarray_plotly.config import _optionsThen update line 358:
if var is None: - from xarray_plotly.config import _options - da = self._ds.to_array(dim="variable") pos = _options.dataset_variable_position
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/examples/datasets.ipynbmkdocs.ymlxarray_plotly/accessor.pyxarray_plotly/config.py
🔇 Additional comments (4)
mkdocs.yml (1)
72-72: LGTM!The navigation entry for the new dataset plotting example is correctly formatted and properly positioned within the Examples section.
xarray_plotly/config.py (1)
61-73: LGTM! Clear documentation and sensible default.The new
dataset_variable_positionoption is well-documented and follows the established pattern for configuration options. The default value of 1 (second position, typically color) is a sensible choice for most use cases.xarray_plotly/accessor.py (1)
352-356: LGTM! Clear documentation of the positioning behavior.The updated docstring clearly explains that the "variable" dimension is positioned according to the configuration option, with appropriate defaults mentioned.
docs/examples/datasets.ipynb (1)
1-260: Excellent comprehensive documentation!The notebook provides clear, well-structured examples demonstrating:
- Basic dataset plotting with automatic variable dimension handling
- Explicit slot assignments for the "variable" dimension
- Configuration of default positioning behavior
- Multiple plot types (line, bar, box, area, scatter, pie)
- Mixing explicit and automatic slot assignments
This will be very helpful for users learning to work with multi-variable datasets.
1. Notebook comment - Clarified that to_array() returns (variable, ...) but xpx() reorders it 2. Negative indexing - Now supports Python-style: -1=last, -2=second-to-last, etc. 3. Import - Moved _options to module level
1 participant
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.