Add Pythonic argument options for colorbar frame parameters#2130
Add Pythonic argument options for colorbar frame parameters#2130willschlitzer wants to merge 16 commits intomainfrom
Conversation
Summary of changed imagesThis is an auto-generated report of images that have changed on the DVC remote
Image diff(s)DetailsAdded images
Modified images
Report last updated at commit 33bdc8f |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I specifically chose to work with
frameincolorbarsince 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 forframewith something likegrdcontourorplot). I think it can stay incolorbarfor 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
xlabelthat 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).
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
| 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 |
There was a problem hiding this comment.
I specifically chose to work with
frameincolorbarsince 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 forframewith something likegrdcontourorplot). I think it can stay incolorbarfor 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
xlabelthat 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).
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
| outs: | ||
| - md5: 7a8bbd25a40214160f00a5c5bf83f69d | ||
| size: 3764 | ||
| path: test_colorbar_frame_string_parameters.png |
There was a problem hiding this comment.
Could you delete this file and see if it fixes the test failure? Maybe it's still comparing the old image?
There was a problem hiding this comment.
Still no luck, tried deleting both the .dvc file and the image.
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
|
@weiji14 I'm not sure why I'm not able to get |
|
Don't think this has much traction, so I'm going to close the issue. |
This adds the parameters
xlabel,ylabel, andannotationto thecolorbarmodule to allow those labels to be set without passing a list toframe. It will still allow other arguments to be passed toframe.This is a proof of concept for moving towards more Pythonic arguments; I feel that
colorbaris pretty straightforward to begin substituting new parameters, as I assume most of the arguments passed toframeaffect either the axis labels or the annotation interval. It should still maintain backward compatibility for setting all of these values withinframe.Addresses #1082
Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.Slash Commands
You can write slash commands (
/command) in the first line of a comment to performspecific operations. Supported slash commands are:
/format: automatically format and lint the code/test-gmt-dev: run full tests on the latest GMT development version