Conversation
… FixedLocator) (#149) * fix tick formatting issue * pycodestyle * copilor reviews
…lpers) and fix single-colorbar/sharex/sharey logic (#151) * update grid position calling * copilot reviews
…d stabilize scatter regression (#152) * address deprecated labels in box and whisker and other fail in Scatter * pycodestyle * copilot reviews
…#150) * update plotting tests * fix errors * pycodestyle * pycodestyle * pycodestyle * minor tweak to scatter linear regression * pycodestyle * handle attribute spellings and public setters correctly * pycodestyle * add fail test for fix in future PR * typo * missing import * missing import * fail safe for another test * update test * update conftest * cleanup * copilot review * fix fixture * missed one
…ing (#153) * Add layer adapter and make changes in renderers * update adapters * revert map adapters * pycodestyle * pycodestyle * pycodestyle * clean up adapters * Address error * add tests for changes made * fix _last_mappable * forgot import * address failing test * fix scatter color issue * another attempt at color error * copilot reviews
…d plotting correctness fixes (#154) * avoid mutable defaults * fix contourf transform on map * clean up invert axis and add tests * tests for invert * strengthen legends * cleanup * add helper in adapters * future proof ticks for GeoAxes * fix deprecated input for boxandwhisker * pycodestyle * Address invert error * pycodestyle * fix test * fix tests again * try again * another test * Attempt invert again * update tests * one more time * remove unnecessary test * quick cleanup * cleanup default arg to avoid mutable defaults * typo * missing import * copilot review
…eprecations (#155) * add more tests to stress test new additions * pynorms * cleanup vmin/vmax norm error * missed import * copilot reviews
…vert) (#157) * add changes to boxplot * copilot reviews
…l `twinx` support (#158) * Add more features * Add tests * add twinx * pycodestyle * update tests * copilot reviews * copilot reviews
* update documentation workflow * removing old files * update new docs main dir * add get-started * add how-to .mds * add explanations .mds * add contributing .mds * update to include gallery * fixes while building html * updates to get running website * clean up site options * add line plot examples to gallery * clean up boxplot features * clean up map errors * more examples * fix all pycodestyle * missed one pycodestyle * fix tests * pycodestyle * pycodestyle * fix class * patch gridded plot * pathc for map_gridded * typos * add new plot types for gallery * update workflow yaml * cleanup * cleanup * rename file * build cleanup * pycodestyle * copilot and Cory reviews
* initial clean up and move .txt to .md files * cleanup readmes for gallery * website builds with zero warnings * avoid cross-call mutations * forgot imports * pynorms * cleans up and prevents norms breaking * pycodestyle * colorbar consistency * add logical validation which raises errors * add more tests * pycodestyle * pynorms * typo * quick fix * remove unsued import * update _norms * pycodestyle * fix * add conditional for strange boundary issue * updates to docs and quick fix * copilot reviews * add import
|
@EdwardSafford-NOAA @ADCollard Cory has been in the loop on all the PRs merged into this branch, I don't expect you to review 100+ files! I wanted to keep you guys in the loop on a major upgrade to EMCPy and that we will need to update all hashes to anything that is dependent. |
There was a problem hiding this comment.
Pull Request Overview
This pull request represents a major refactor of EMCPy, introducing a new architecture for plot layer rendering, enhanced test coverage, and extensive documentation improvements. The changes modernize the codebase while maintaining backward compatibility for existing APIs.
Key changes include:
- Introduced an adapter pattern for plot layer rendering with centralized registry
- Added new plot layers: FillBetween, ErrorBar, ViolinPlot, HexBin, and Hist2D
- Refactored box-and-whisker plots with improved orientation handling and Matplotlib version compatibility
- Enhanced discrete field support with automatic BoundaryNorm generation for integer categories
- Comprehensive test suite restructure with improved CI configuration
- Documentation overhaul with new gallery examples and explanations sections
Reviewed Changes
Copilot reviewed 95 out of 102 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/test_plots.py | Removed legacy test file (729 lines deleted) |
| src/tests/test_map_plots.py | Removed legacy map test file (235 lines deleted) |
| src/tests/plotting/test_*.py | Added comprehensive new test suite covering ticks, maps, colorbars, adapters, and layer functionality |
| src/emcpy/plots/plots.py | Enhanced with new layer classes and improved BoxandWhiskerPlot compatibility |
| src/emcpy/plots/map_plots.py | Strengthened validation and error handling for map layers |
| src/emcpy/plots/create_plots.py | Major refactor introducing adapter pattern and enhanced rendering pipeline |
| src/emcpy/plots/adapters.py | New adapter registry system for plot layer rendering |
| galleries/ | Added extensive example gallery with statistical, map, and gridded plot examples |
| docs/ | Complete documentation restructure with modern Sphinx configuration |
| pyproject.toml | Modern Python packaging configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/emcpy/plots/plots.py
Outdated
| self.edgecolors = None | ||
| self.label = f'n={np.count_nonzero(~np.isnan(x))}' | ||
| self.do_linear_regression = False | ||
| # Optional style overrides for the regression line; kept empty by default. |
There was a problem hiding this comment.
The comment mentions 'kept empty by default' but the code initializes it to an empty dict. The comment should clarify that it's initialized as an empty dictionary to store optional style overrides.
| # Optional style overrides for the regression line; kept empty by default. | |
| # Optional style overrides for the regression line, initialized as an empty dictionary. |
| """ | ||
| Sets extent, longitude xticks, and latitude yticks | ||
| for arctic domain. | ||
| for antarctic domain. |
There was a problem hiding this comment.
The comment mentions 'for arctic domain' in the docstring but this function is _south() which should be for 'antarctic domain'.
src/emcpy/plots/map_plots.py
Outdated
| raise ValueError( | ||
| "MapScatter: latitude values exceed 90°, which suggests you passed " | ||
| "longitude first. Constructor order is (latitude, longitude, data)." |
There was a problem hiding this comment.
This validation logic for checking swapped lat/lon parameters is repeated in multiple classes (MapScatter, MapGridded, MapContour, MapFilledContour). Consider extracting this into a shared validation utility function to reduce code duplication.
| ) -> None: | ||
|
|
There was a problem hiding this comment.
The function creates defensive copies of stats_dict and kwargs but doesn't document why this is necessary. Consider adding a docstring explaining the parameter copying behavior to avoid confusion about mutable default arguments.
| ) -> None: | |
| ) -> None: | |
| """ | |
| Add a dictionary of statistics to the plot, with location and formatting options. | |
| Defensive copies of `stats_dict` and `kwargs` are made to avoid accidental mutation | |
| of the input arguments after this method is called. This ensures that changes to the | |
| original dictionaries outside this method do not affect the stored statistics or their | |
| formatting in the plot. | |
| Parameters | |
| ---------- | |
| stats_dict : Optional[Mapping[str, Any]] | |
| Dictionary of statistics to display. A copy is made internally. | |
| xloc : float, default 0.5 | |
| X location for the statistics text. | |
| yloc : float, default -0.1 | |
| Y location for the statistics text. | |
| ha : str, default "center" | |
| Horizontal alignment. | |
| **kwargs : Any | |
| Additional keyword arguments for formatting. A copy is made internally. | |
| """ |
src/emcpy/plots/create_plots.py
Outdated
| import numpy as _np # local import to avoid any surprises at import time | ||
| return isinstance(v, (bool, _np.bool_)) and bool(v) |
There was a problem hiding this comment.
The local import of numpy as _np inside a nested function is unusual. Consider importing numpy at the module level or documenting why the local import is necessary to avoid 'surprises at import time'.
| import numpy as _np # local import to avoid any surprises at import time | |
| return isinstance(v, (bool, _np.bool_)) and bool(v) | |
| return isinstance(v, (bool, np.bool_)) and bool(v) |
| groups = [rng.normal(0.0, 1.0, 400), | ||
| rng.normal(0.5, 0.8, 400), | ||
| rng.normal(-0.3, 1.2, 400)] |
There was a problem hiding this comment.
[nitpick] The gallery example uses hard-coded array sizes (400). Consider using a variable like n_samples = 400 to make the code more maintainable and self-documenting.
| groups = [rng.normal(0.0, 1.0, 400), | |
| rng.normal(0.5, 0.8, 400), | |
| rng.normal(-0.3, 1.2, 400)] | |
| n_samples = 400 | |
| groups = [rng.normal(0.0, 1.0, n_samples), | |
| rng.normal(0.5, 0.8, n_samples), | |
| rng.normal(-0.3, 1.2, n_samples)] |
CoryMartin-NOAA
left a comment
There was a problem hiding this comment.
Herculean effort, @kevindougherty-noaa ! Nice work!
This pull request merges several recent enhancements, refactors, and documentation improvements into a single cohesive update. It represents a major step toward making EMCPy more robust, extensible, and consistent across use cases.
Highlights
FillBetween,ErrorBar,Violin,HexBin, andHist2D.Breaking Changes / Migration Notes
FillBetween) no longer accept certain kwargs (alphamust now be set via attributes after initialization).orientation="vertical"|"horizontal"; thevertflag is no longer passed directly to Matplotlib.integer_field=Truehas been standardized; results may differ slightly in tick placement or colorbar labeling compared to older versions.Impact
This PR lays the foundation for EMCPy v2 by improving stability, expanding plotting options, and streamlining the developer experience. It enables easier extension of plotting features while maintaining a consistent user interface.