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

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Sep 21, 2022

This adds the parameters xlabel, ylabel, and annotation to the colorbar module to allow those labels to be set without passing a list to frame. It will still allow other arguments to be passed to frame.

This is a proof of concept for moving towards more Pythonic arguments; I feel that colorbar is pretty straightforward to begin substituting new parameters, as I assume most of the arguments passed to frame affect either the axis labels or the annotation interval. It should still maintain backward compatibility for setting all of these values within frame.

Addresses #1082

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@willschlitzer willschlitzer added the enhancement Improving an existing feature label Sep 21, 2022
@willschlitzer willschlitzer self-assigned this Sep 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2022

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
added pygmt/tests/baseline/test_colorbar_frame_list_parameters.png
added pygmt/tests/baseline/test_colorbar_frame_parameters.png
added pygmt/tests/baseline/test_colorbar_frame_string_parameters.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_colorbar_frame_list_parameters.png

  • pygmt/tests/baseline/test_colorbar_frame_parameters.png

  • pygmt/tests/baseline/test_colorbar_frame_string_parameters.png

Modified images

Path Old New

Report last updated at commit 33bdc8f

@willschlitzer willschlitzer marked this pull request as draft September 21, 2022 19:32
@willschlitzer willschlitzer marked this pull request as ready for review September 22, 2022 17:20
pygmt/src/colorbar.py Outdated Show resolved Hide resolved
Comment on lines +114 to +128
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
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).

pygmt/tests/test_colorbar.py Show resolved Hide resolved
pygmt/tests/test_colorbar.py Outdated Show resolved Hide resolved
Comment on lines +114 to +128
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
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).

Comment on lines +1 to +4
outs:
- md5: 7a8bbd25a40214160f00a5c5bf83f69d
size: 3764
path: test_colorbar_frame_string_parameters.png
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.

@willschlitzer
Copy link
Contributor Author

@weiji14 I'm not sure why I'm not able to get test_colorbar_frame_parameters and test_colorbar_frame_string_parameters to match up. I'm going to leave it as two separate reference images.

@willschlitzer willschlitzer requested a review from a team December 1, 2022 13:01
@seisman seisman added this to the 0.9.0 milestone Dec 20, 2022
@willschlitzer willschlitzer added the needs review This PR has higher priority and needs review. label Jan 8, 2023
@maxrjones maxrjones mentioned this pull request Mar 14, 2023
36 tasks
@seisman seisman modified the milestones: 0.9.0, 0.10.0 Mar 19, 2023
@willschlitzer
Copy link
Contributor Author

Don't think this has much traction, so I'm going to close the issue.

@seisman seisman removed the needs review This PR has higher priority and needs review. label Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
Development

Successfully merging this pull request may close these issues.

3 participants