Allow passing None explicitly to pygmt functions Part 1 by weiji14 · Pull Request #1857 · GenericMappingTools/pygmt

weiji14 · 2022-03-30T14:33:36Z

Description of proposed changes

Several PyGMT functions have None as the default value (as seen in the docstring), but if a user were to explicitly set a None argument, this can fail. This Pull Request implements a more robust check for None values, as discussed in #1665 (comment)

For example, in the case of grdimage, instead of checking if "I" in kwargs, it is usually better to do if kwargs.get("I") is not None.

TODO:

Fixes #1852, closes #1807

⚠️ Fair warning to reviewers: prepare to get a headache from thinking in double negatives.

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

Refactor grdimage to check `if "I" in kwargs` to using `if kwargs.get("I") is not None`.

Specifically when output_type="pandas" too.

Or rather, catch it properly if someone uses spec=None.

weiji14 · 2022-03-30T18:20:00Z

I think I'll stop here for now to keep this PR fairly 'small'. There's still a list of other functions that need to be refactored to use .get(). Search for " not in kwargs and you'll find this list:

Would someone else be willing to open a Part 2 PR for this?

pygmt/src/grd2cpt.py

pygmt/src/makecpt.py

maxrjones · 2022-03-30T19:49:34Z

Would someone else be willing to open a Part 2 PR for this?

I could open part 2.

weiji14 · 2022-03-30T20:35:09Z

Would someone else be willing to open a Part 2 PR for this?

I could open part 2.

Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 😉

maxrjones · 2022-03-30T21:19:14Z

Would someone else be willing to open a Part 2 PR for this?

I could open part 2.

Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 😉

Should I open the PR before or after this one is approved/merged?

weiji14 · 2022-03-30T21:43:29Z

Would someone else be willing to open a Part 2 PR for this?

I could open part 2.

Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 wink

Should I open the PR before or after this one is approved/merged?

You can open the Part 2 PR now if you want, I think there shouldn't be any conflicts. Probably better to work on both in parallel too in case we notice anything wrong with the implementation in one or the other.

pygmt/src/grdview.py

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu> Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

weiji14 · 2022-03-31T14:39:57Z

/format

weiji14 · 2022-04-01T14:03:16Z

Just to be safe, let's wait until the Part 2 PR is opened and finalized before merging this into the main branch. I'll keep this PR in draft for now.

maxrjones · 2022-04-01T14:56:06Z

Just to be safe, let's wait until the Part 2 PR is opened and finalized before merging this into the main branch. I'll keep this PR in draft for now.

Working on it now!

maxrjones · 2022-04-01T15:13:06Z

I am leaving meca out of part 2, since it is undergoing a complete refactor in #1784.

maxrjones · 2022-04-01T15:26:12Z

pygmt/src/velo.py

@@ -238,8 +238,12 @@ def velo(self, data=None, **kwargs):
    """
    kwargs = self._preprocess(**kwargs)  # pylint: disable=protected-access


This isn't really related to the PR, but it's something that I found confusing while working on part 2. Why is kwargs passed to figure._preprocess? It doesn't look like it is used at all.

Good question, this ensures that velo is plotted to the right instance of the figure (e.g. if someone does fig1, fig2, etc). See also #1072 (comment). The _preprocess code is here:

pygmt/pygmt/figure.py

Lines 109 to 115 in 0aa04d7

def _preprocess(self, **kwargs):

"""

Call the ``figure`` module before each plotting command to ensure we're

plotting to this particular figure.

"""

self._activate_figure()

return kwargs

It makes sense to me why we call the figure method to ensure the correct figure is activated in the session. I still don't really understand why we pass and return kwargs to/from _preprocess, or even why we call _preprocess at all rather than calling _activate_figure directly. I pushed a new branch to demonstrate the point of confusion, in which I replaced all the kwargs = self._preprocess(**kwargs) with self._activate_figure which seems more readable, a tiny bit faster, and still seems to work fine.

This was added in #148, I'm not sure exactly why, but maybe @leouieda was thinking that there might be some other common function that needs to be called every time a plotting function happens.

This was setup for any other preprocessing of kwargs that may be needed or other figure plotting methods. At the time, I put this in place to allow things like passing pygmt.Figure(projection=pygmt.Mercator(), width="25c") and then inserting the projection code into the first plot command.

It could also be useful for any bookkeeping operation that is needed, like activating the figure, activating a subplot, etc.

maxrjones · 2022-04-04T15:51:53Z

pygmt/src/grdgradient.py

+                if kwargs.get("G") is None:  # if outgrid is unset, output to tempfile
                    kwargs.update({"G": tmpfile.name})
                outgrid = kwargs["G"]


Suggested change

if kwargs.get("G") is None: # if outgrid is unset, output to tempfile

kwargs.update({"G": tmpfile.name})

outgrid = kwargs["G"]

if (outgrid := kwargs.get("G")) is None:

kwargs["G"] = outgrid = tmpfile.name # output to tmpfile

I think this is slightly more readable (inspired by item number 16 in Effective Python (2021)).

Ok, my brain is telling me that there's a reason not to do variable assignment like a = b = "c", something about how changing b would also change a but after some testing, it seems to be ok. Maybe I'm just thinking of some other programming language I've used before like Matlab, Javascript or Python 2 🤷

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

seisman · 2022-04-06T12:56:29Z

Search for in kwargs, I still see some functions that use in kwargs. Any specific reasons that these functions are not updated?

$ grep 'in kwargs' */**/*.py                                                                                                                                                                        pygmt/clib/conversion.py:    If the argument is not present in kwargs, returns ``None``.
pygmt/clib/conversion.py:    if argument in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/datasets/samples.py:    if "suppress_warning" not in kwargs:
pygmt/figure.py:        if "A" not in kwargs:
pygmt/figure.py:            if "N" not in kwargs:
pygmt/helpers/decorators.py:                if long_alias in kwargs and short_param in kwargs:
pygmt/helpers/decorators.py:                if long_alias in kwargs:
pygmt/helpers/decorators.py:                elif short_param in kwargs:
pygmt/helpers/decorators.py:                if arg in kwargs:
pygmt/helpers/decorators.py:            if oldname in kwargs:
pygmt/helpers/decorators.py:                if newname in kwargs:
pygmt/io.py:    if "cache" in kwargs:
pygmt/src/config.py:            for key in kwargs:
pygmt/src/config.py:        arg_str = " ".join([f'{key}="{value}"' for key, value in kwargs.items()])
pygmt/src/grdclip.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdcut.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdfill.py:    if "A" not in kwargs and "L" not in kwargs:
pygmt/src/grdfill.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdfilter.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdlandmask.py:    if "I" not in kwargs or "R" not in kwargs:
pygmt/src/grdlandmask.py:            if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdproject.py:    if "J" not in kwargs:
pygmt/src/grdproject.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/grdsample.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/info.py:        if any(arg in kwargs for arg in ["C", "I", "T"]):
pygmt/src/legend.py:    if "D" not in kwargs:
pygmt/src/legend.py:        if "F" not in kwargs:
pygmt/src/meca.py:                    if "A" not in kwargs:
pygmt/src/meca.py:                        if "A" not in kwargs:
pygmt/src/meca.py:                        if "A" not in kwargs:
pygmt/src/sph2grd.py:                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
pygmt/src/sphdistance.py:    if "I" not in kwargs or "R" not in kwargs:
pygmt/src/xyz2grd.py:    if "I" not in kwargs or "R" not in kwargs:

weiji14 · 2022-04-06T13:20:20Z

Search for in kwargs, I still see some functions that use in kwargs. Any specific reasons that these functions are not updated?

Oops, not sure how I missed all of that. Looks like we need a Part 3 😅 Do you want to open up that PR @seisman?

seisman · 2022-04-06T14:42:11Z

Search for in kwargs, I still see some functions that use in kwargs. Any specific reasons that these functions are not updated?

Oops, not sure how I missed all of that. Looks like we need a Part 3 😅 Do you want to open up that PR @seisman?

OK, I'll open a PR later.

…ngTools#1857) Implements a more robust check for None values in pygmt functions. * Let grdimage shading=None or False work Refactor grdimage to check `if "I" in kwargs` to using `if kwargs.get("I") is not None`. * Let grd2cpt's categorical, cyclic and output work with None input * Let grd2xyz's outcols work with None input Specifically when output_type="pandas" too. * Let grdgradient's tiles, normalize and outgrid work with None input * Let grdview's drapegrid work with None inputs * Let makecpt's categorical, cyclic and output work with None inputs * Let plot's style, color, intensity and transparency work with None input * Let plot3d's style, color, intensity & transparency work with None input * Let solar's T work with None input * Let transparency work with 0, None and False input * Let project's center, convention and generate work with None inputs * Let velo's spec work with None inputs Or rather, catch it properly if someone uses spec=None. * Update pygmt/src/grdgradient.py using walrus operator Co-authored-by: Meghan Jones <meghanj@alum.mit.edu> Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

Let grdimage shading=None or False work

3cd0540

Refactor grdimage to check `if "I" in kwargs` to using `if kwargs.get("I") is not None`.

weiji14 added the bug Something isn't working label Mar 30, 2022

weiji14 added this to the 0.6.1 milestone Mar 30, 2022

weiji14 self-assigned this Mar 30, 2022

weiji14 mentioned this pull request Mar 30, 2022

Release PyGMT v0.6.1 #1856

Closed

30 tasks

weiji14 changed the title ~~Let grdimage shading=None or False work~~ Allow passing None or False explicitly to pygmt functions Mar 30, 2022

Let grd2cpt's categorical, cyclic and output work with None input

3b722e6

weiji14 changed the title ~~Allow passing None or False explicitly to pygmt functions~~ Allow passing None explicitly to pygmt functions Mar 30, 2022

weiji14 added 2 commits March 30, 2022 10:52

Let grd2xyz's outcols work with None input

7b10bd2

Specifically when output_type="pandas" too.

Let grdgradient's tiles, normalize and outgrid work with None input

2e58272

vercel bot temporarily deployed to Preview March 30, 2022 15:43 Inactive

weiji14 added 4 commits March 30, 2022 12:46

Let grdview's drapegrid work with None inputs

8440848

Let makecpt's categorical, cyclic and output work with None inputs

e1b71d2

Let plot's style, color, intensity and transparency work with None input

2d78667

Let plot3d's style, color, intensity & transparency work with None input

900273b

vercel bot temporarily deployed to Preview March 30, 2022 17:00 Inactive

weiji14 added 4 commits March 30, 2022 13:44

Let solar's T work with None input

f1e8d37

Let transparency work with 0, None and False input

8d8f138

Let project's center, convention and generate work with None inputs

58390de

Let velo's spec work with None inputs

bc09dcb

Or rather, catch it properly if someone uses spec=None.

vercel bot temporarily deployed to Preview March 30, 2022 18:14 Inactive

weiji14 changed the title ~~Allow passing None explicitly to pygmt functions~~ Allow passing None explicitly to pygmt functions Part 1 Mar 30, 2022

maxrjones reviewed Mar 30, 2022

View reviewed changes

pygmt/src/grd2cpt.py Outdated Show resolved Hide resolved

pygmt/src/makecpt.py Outdated Show resolved Hide resolved

seisman reviewed Mar 31, 2022

View reviewed changes

pygmt/src/grdview.py Outdated Show resolved Hide resolved

Apply suggestions from code review

56fee1f

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu> Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

vercel bot temporarily deployed to Preview March 31, 2022 14:38 Inactive

[format-command] fixes

b7fd366

vercel bot temporarily deployed to Preview March 31, 2022 14:54 Inactive

seisman approved these changes Apr 1, 2022

View reviewed changes

weiji14 added the final review call This PR requires final review and approval from a second reviewer label Apr 1, 2022

maxrjones reviewed Apr 1, 2022

View reviewed changes

maxrjones mentioned this pull request Apr 1, 2022

Allow passing None explicitly to pygmt functions Part 2 #1862

Merged

6 tasks

seisman mentioned this pull request Apr 3, 2022

Refactor build_arg_string to also deal with infile and outfile #1837

Merged

6 tasks

maxrjones reviewed Apr 4, 2022

View reviewed changes

Update pygmt/src/grdgradient.py using walrus operator

c7725e0

Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>

vercel bot temporarily deployed to Preview April 4, 2022 17:24 Inactive

Merge branch 'main' into fix/arg_is_not_None

a8f5705

vercel bot temporarily deployed to Preview April 4, 2022 17:38 Inactive

weiji14 marked this pull request as ready for review April 4, 2022 19:48

maxrjones approved these changes Apr 4, 2022

View reviewed changes

weiji14 enabled auto-merge (squash) April 4, 2022 19:57

weiji14 merged commit 722dae9 into main Apr 4, 2022

weiji14 deleted the fix/arg_is_not_None branch April 4, 2022 20:04

weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Apr 4, 2022

seisman mentioned this pull request Apr 6, 2022

Allow passing None explicitly to pygmt functions Part 3 #1872

Merged

6 tasks

seisman mentioned this pull request Aug 27, 2022

Correct and consistent usage of 'function, 'method', and 'class' #2043

Merged

23 tasks

		@@ -238,8 +238,12 @@ def velo(self, data=None, **kwargs):
		"""
		kwargs = self._preprocess(**kwargs) # pylint: disable=protected-access

	def _preprocess(self, **kwargs):
	"""
	Call the ``figure`` module before each plotting command to ensure we're
	plotting to this particular figure.
	"""
	self._activate_figure()
	return kwargs

Conversation

weiji14 commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weiji14 commented Mar 30, 2022

Uh oh!

Uh oh!

Uh oh!

maxrjones commented Mar 30, 2022

Uh oh!

weiji14 commented Mar 30, 2022

Uh oh!

maxrjones commented Mar 30, 2022

Uh oh!

weiji14 commented Mar 30, 2022

Uh oh!

Uh oh!

weiji14 commented Mar 31, 2022

Uh oh!

weiji14 commented Apr 1, 2022

Uh oh!

maxrjones commented Apr 1, 2022

Uh oh!

maxrjones commented Apr 1, 2022

Uh oh!

maxrjones Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

weiji14 Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxrjones Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

weiji14 Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

leouieda May 4, 2022

Choose a reason for hiding this comment

Uh oh!

maxrjones Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

weiji14 Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

seisman commented Apr 6, 2022

Uh oh!

weiji14 commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seisman commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

weiji14 commented Mar 30, 2022 •

edited

Loading

weiji14 Apr 1, 2022 •

edited

Loading

weiji14 commented Apr 6, 2022 •

edited

Loading