Update and standardize all top-level function signatures and docstrings #493
Conversation
| artist_number: int = 0, | ||
| errorbars: bool = True, | ||
| mask_color: str = 'black', | ||
| mask_color: str | None = None, |
There was a problem hiding this comment.
Because the thing that creates Line has None by default, so if I leave the default value here to black, it will receive None by default and I still need to handle the case where mask_color is None further down in the function.
So I thought it makes more sense to have None as the default here as well.
There was a problem hiding this comment.
Pull Request Overview
This PR updates function signatures and docstrings across all top-level plotting functions to standardize parameters and improve documentation consistency. The changes replace the generic **kwargs approach with explicit parameter definitions for better type safety and user experience.
- Standardized parameter signatures with explicit type hints across all plotting functions
- Enhanced docstrings with comprehensive parameter descriptions and proper formatting
- Updated test cases to use keyword-based function calls instead of positional arguments
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/plotting/scatter_test.py |
Added test for position parameter and updated test data usage |
tests/graphics/artists_test.py |
Converted test cases from positional to keyword arguments |
src/plopp/plotting/xyplot.py |
Added explicit parameters and comprehensive docstring |
src/plopp/plotting/superplot.py |
Standardized function signature with explicit parameters |
src/plopp/plotting/slicer.py |
Enhanced parameter definitions and docstring documentation |
src/plopp/plotting/scatter3d.py |
Reordered and added parameters with improved documentation |
src/plopp/plotting/scatter.py |
Added position parameter and standardized function signature |
src/plopp/plotting/plot.py |
Refactored to use categorize_args helper function |
src/plopp/plotting/mesh3d.py |
Updated parameter order and enhanced docstring |
src/plopp/plotting/inspector.py |
Added explicit parameters for both 1D and 2D figure configurations |
src/plopp/plotting/common.py |
Added categorize_args helper function and renamed check function |
src/plopp/graphics/graphicalview.py |
Added mask_color parameter support |
src/plopp/graphics/colormapper.py |
Enhanced mask color handling logic |
src/plopp/backends/pythreejs/scatter3d.py |
Added mask_color parameter |
src/plopp/backends/pythreejs/mesh3d.py |
Added ignored parameters handling |
src/plopp/backends/matplotlib/scatter.py |
Updated mask_color parameter handling |
src/plopp/backends/matplotlib/line.py |
Enhanced mask_color parameter with default handling |
Comments suppressed due to low confidence (1)
tests/plotting/scatter_test.py:41
- The new test function
test_scatter_pos()only verifies that the function call doesn't raise an exception but doesn't assert any specific behavior or validate the output.
a = scatter_data()
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| obj: PlottableMulti, | ||
| x: str, | ||
| y: str, | ||
| pos: str | None, | ||
| size: str | None, | ||
| name: str | None = None, | ||
| ignore_size: bool = False, | ||
| ): |
There was a problem hiding this comment.
The function signature change adds a required pos parameter but the function call sites may not be updated to provide this parameter, potentially causing breaking changes.
| CASES = { | ||
| "line-mpl-static": (('2d', 'mpl-static'), pp.plot, (pp.data.data1d(),)), | ||
| "line-mpl-interactive": (('2d', 'mpl-interactive'), pp.plot, (pp.data.data1d(),)), | ||
| "line-mpl-static": (('2d', 'mpl-static'), pp.plot, {'obj': pp.data.data1d()}), |
There was a problem hiding this comment.
Changes in this file are because mesh3d no longer accepts positional arguments
| sc.DimensionError, match='Scatter only accepts data with 1 dimension' | ||
| ): | ||
| fig.update(new=data_array(ndim=2, dim_list="xyzab")) | ||
| fig.update(new=da.fold(dim=da.dim, sizes={'a': 2, 'b': -1})) |
There was a problem hiding this comment.
It seems that this test was sort of broken. It was getting data with an incompatible unit, as well as incompatible dims.
Alternative to #492.
We just update everything manually here.
It's more difficult to maintain, but means that we can still use Mypy and is more customizable.