Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated fill docstring currently contradicts the implemented/tested behavior for CPT-based fills and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates Figure.histogram to support a more Pythonic way to request CPT-based bar filling by extending the existing fill parameter to accept special values ("position" and "value"), while keeping the legacy cmap (-C) syntax working.
Changes:
- Extend
histogram(fill=...)to accept"position"/"value"and translate them into the appropriate-C/-C+bCLI behavior. - Add image-based regression tests for the new
fillsyntax and the legacycmapsyntax. - Add a new baseline image for the histogram fill behavior.
File summaries
| File | Description |
|---|---|
pygmt/src/histogram.py |
Implements the new fill="position"/fill="value" syntax and maps it onto -C / -C+b. |
pygmt/tests/test_histogram.py |
Adds pytest-mpl image tests covering both new and legacy CPT fill syntaxes, plus an error case. |
pygmt/tests/baseline/test_histogram_fill.png |
Adds the new baseline image used by the pytest-mpl tests. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
| - ``"position"``: look up the color using the mid-coordinate of the bin. This is | ||
| the default when ``cmap`` is set. |
There was a problem hiding this comment.
Here we write "bin", but the GMT says "bar", please see https://docs.generic-mapping-tools.org/dev/histogram.html#c:
Give a CPT. The mid-coordinate for each bar is used to look up the bar color.
I agree that "bin" makes more sense. The x-position of the bars can be changed, e.g., via bar_offset.
There was a problem hiding this comment.
I think "bar" represents the visual element, while "bin" represents the actual data range of a bar.
In
histogram, bars can be filled by a constant color/pattern, or by the bin position or value (count, frequency). This is achieved by the-Coption, with a syntax like-C<cmap>+b.This PR implements a Pythonic syntax for such features by extending the existing parameter
fill(alias of-G). In addition to color or pattern, nowfillcan accept two special values:"position"and"value".GMT CLI and PyGMT comparison
Here is a comparison:
-Gredfill="red"-Gp5fill=Pattern(5)-Cfill="position"-Ctest.cptfill="position", cmap="test.cpt"test.cpt-C+bfill="value"-Ctest.cpt+bfill="value", cmap="test.cpt"test.cptExamples
Alternative implementation
An alternative implementation is to keep
fillunchanged, but alias-Cto two separate parameters,cmapandfill_by_value(or any other names). The cons are:fillandfill_by_value-Gcan't be used with-C, so we need to deal with conflicts betweenfillandcmap/fill_by_valuecarefullyAddress #4251