Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pythonic argument options for colorbar frame parameters #2130

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion pygmt/src/colorbar.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
@kwargs_to_strings(
R="sequence", G="sequence", I="sequence", c="sequence_comma", p="sequence"
)
def colorbar(self, **kwargs):
def colorbar(self, frame=None, annotation=None, xlabel=None, ylabel=None, **kwargs):
r"""
Plot a gray or color scale-bar on maps.

Expand All @@ -45,6 +45,12 @@ def colorbar(self, **kwargs):
----------
frame : str or list
Set color bar boundary frame, labels, and axes attributes.
annotation : int or float or str
Set the interval for annotated gridlines on the colorbar.
xlabel : str
Set the label for the x-axis of the colorbar.
ylabel : str
Set the label for the y-axis of the colorbar.
{cmap}
position : str
[**g**\|\ **j**\|\ **J**\|\ **n**\|\ **x**]\ *refpoint*\
Expand Down Expand Up @@ -102,6 +108,21 @@ def colorbar(self, **kwargs):
{perspective}
{transparency}
"""
if xlabel or ylabel or annotation:
if kwargs.get("B") is None:
frame = []
elif isinstance(kwargs.get("B"), list):
frame = kwargs.get("B")
else:
frame = [kwargs.get("B")]
if xlabel:
frame.append("x+l" + str(xlabel))
if ylabel:
frame.append("y+l" + str(ylabel))
if annotation:
frame.append("a" + str(annotation))
if frame:
kwargs["B"] = frame
Comment on lines +111 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice backward compatible way of handling old frame/B arguments while introducing new user-friendly parameters. I understand that putting the logic here in colorbar is a good proof of concept. Just want to ask though, what's your medium term plan for this chunk of if-then logic? Specifically, do you think we should duplicate this for every PyGMT module that does frame/B, or put it somewhere central (e.g. as a decorator under https://github.com/GenericMappingTools/pygmt/blob/v0.7.0/pygmt/helpers/decorators.py) or something else?

I don't want to resurface the discussion in #1082 on dictionary/functions/classes too much, since nothing would get done if just keep talking while not doing anything. But a colleague of mine recently mentioned that using the function based approach in PyGMT (i.e. passing in parameter-arguments like xlabel="Some label") seemed like a good idea, so I can be swayed with going for this approach rather than the over-engineered class-based approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would make sense to have the logic in something like decorators.py and am open to suggestions. I specifically chose to work with frame in colorbar since there aren't a ton of edge cases to support, and it doesn't need to be consistent with other modules (as opposed to adding this logic for frame with something like grdcontour or plot). I think it can stay in colorbar for now, but I don't feel that strongly.

I don't think I understand the function based approach method you mention; it looks to me like there would still be a parameter xlabel that has an argument passed to it. I'm definitely on board with other approaches; I just want to create proofs of concepts to get the ball rolling on more Pythonic arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specifically chose to work with frame in colorbar since there aren't a ton of edge cases to support, and it doesn't need to be consistent with other modules (as opposed to adding this logic for frame with something like grdcontour or plot). I think it can stay in colorbar for now, but I don't feel that strongly.

Agree to keep the logic in colorbar for now. The key part is the unit tests really (which tests what the users will be doing). The implementation, be it as a decorator or some if-then check here can change afterwards.

I don't think I understand the function based approach method you mention; it looks to me like there would still be a parameter xlabel that has an argument passed to it. I'm definitely on board with other approaches; I just want to create proofs of concepts to get the ball rolling on more Pythonic arguments.

By function-based approach, I meant exactly what you're doing here - doing fig.colorbar(..., xlabel="something"). Contrast this with the class-based approach I mentioned in #1082 (comment) that would be like:

frame = pygmt.param.Frame(xlabel="something")
fig.colorbar(..., frame=frame)

Function-based approach would be more user friendly (allows for tab-completion). Class-based approach would be more developer friendly (less coding for us).

kwargs = self._preprocess(**kwargs) # pylint: disable=protected-access
with Session() as lib:
lib.call_module(module="colorbar", args=build_arg_string(kwargs))
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
outs:
- md5: 69740a2512fd502e3d53d16bddc0f97c
size: 4878
path: test_colorbar_frame_list_parameters.png
4 changes: 4 additions & 0 deletions pygmt/tests/baseline/test_colorbar_frame_parameters.png.dvc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
outs:
- md5: b7fc503b09c1c610a22081edf9cc612f
size: 4840
path: test_colorbar_frame_parameters.png
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
outs:
- md5: 7a8bbd25a40214160f00a5c5bf83f69d
size: 3764
path: test_colorbar_frame_string_parameters.png
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you delete this file and see if it fixes the test failure? Maybe it's still comparing the old image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no luck, tried deleting both the .dvc file and the image.

50 changes: 50 additions & 0 deletions pygmt/tests/test_colorbar.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,53 @@ def test_colorbar_shading_list():
fig.basemap(region=[0, 10, 0, 10], projection="X15c", frame="a")
fig.colorbar(cmap="geo", shading=[-0.7, 0.2], frame=True)
return fig


@pytest.mark.mpl_image_compare
def test_colorbar_frame_parameters():
"""
Create colorbar with arguments for xlabel, ylabel, and annotation, and no
argument passed to frame.
"""
fig = Figure()
fig.colorbar(
cmap="rainbow",
position="x0c/0c+w1c/0.5c",
xlabel="test-x",
ylabel="test-y",
annotation=0.25,
)
return fig


@pytest.mark.mpl_image_compare
def test_colorbar_frame_list_parameters():
"""
Create colorbar with a list passed to frame and an argument passed to
xlabel.
"""
fig = Figure()
fig.colorbar(
cmap="rainbow",
position="x0c/0c+w1c/0.5c",
frame=["a.25", "y+lylabel"],
xlabel="test-x",
)
return fig


@pytest.mark.mpl_image_compare
def test_colorbar_frame_string_parameters():
willschlitzer marked this conversation as resolved.
Show resolved Hide resolved
"""
Create colorbar with a string passed to frame and arguments passed to
xlabel and ylabel.
"""
fig = Figure()
fig.colorbar(
cmap="rainbow",
position="x0c/0c+w1c/0.5c",
frame="a0.5",
willschlitzer marked this conversation as resolved.
Show resolved Hide resolved
xlabel="test-x",
ylabel="test-y",
)
return fig