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

Support tab completion in Jupyter by inserting aliases into the method signature #1282

Merged
merged 14 commits into from Jun 16, 2021

Conversation

maxrjones
Copy link
Member

@maxrjones maxrjones commented May 21, 2021

Description of proposed changes

This PR adds a decorator that adds aliases as parameters to the signature of PyGMT methods to support tab completion. It is motivated by this suggestion, but with a different implementation. The decorator was added to blockmean and blockmedian as examples. (edit: the decorator was added to all methods that use use_alias, but this can easily be reverted if needed). Here is what the result looks like in the docs and a jupyter notebook:
image
image

I tested that tab completing the alias keywords works in a Jupyter notebook, but not in any other environments.

Addresses #1203

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 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

@maxrjones maxrjones marked this pull request as draft May 21, 2021 21:07
@seisman
Copy link
Member

seisman commented May 23, 2021

Great work! For me, it works well in IPython console, Jupyter notebook, but not in VSCode.

@@ -327,6 +328,30 @@ def new_module(*args, **kwargs):
return alias_decorator


def insert_alias(module_func):
Copy link
Member

@seisman seisman May 23, 2021

Choose a reason for hiding this comment

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

Perhaps we should move the code into the use_alias decorator, because we only need to "insert aliases into the signature" when aliases are defined by @use_alias? It also means all functions support tab completion without any further actions (i.e., adding @insert_alias).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that combinging use_alias and insert_alias is the smart move, since I don't see how insert_alias would be used on its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea of implementing this inside use_alias to simplify the diff, but would prefer to not combine by simply moving the code inside use_alias. As an alternate solution, my latest commit calls insert_alias from within use_alias. There are two reasons why I prefer this option to having all the code within use_alias. First, I think that it is simpler (i.e., easier to maintain) if individual functions do smaller, isolated tasks. Second, this implementation is compatible with the changes proposed in #1282. Even if that PR doesn't get merged, it seems to me that having insert_alias as a separate function could be more compatible with future updates to the decorators (reason 1).

@maxrjones
Copy link
Member Author

I learned that parameter order matters for tab completion when trying this out. For example, I can tab complete each parameter in this use of basemap:

import pygmt
fig = pygmt.Figure()
fig.basemap(region=[-180,180,-80,80],projection="M6i",frame=True)

but cannot tab complete region if I select the projection parameter first:

import pygmt
fig = pygmt.Figure()
fig.basemap(projection="M6i",region=[-180,180,-80,80],frame=True)

It seems like this is the same for other libraries in Jupyter notebooks (e.g., numpy), so I am not sure if there is a way around this but will look into it more. If not, it motivates ordering the aliases in some way that reflects a likely usage order as much as possible.

@seisman
Copy link
Member

seisman commented May 23, 2021

I learned that parameter order matters for tab completion when trying this out. For example, I can tab complete each parameter in this use of basemap:

import pygmt
fig = pygmt.Figure()
fig.basemap(region=[-180,180,-80,80],projection="M6i",frame=True)

but cannot tab complete region if I select the projection parameter first:

import pygmt
fig = pygmt.Figure()
fig.basemap(projection="M6i",region=[-180,180,-80,80],frame=True)

It seems like this is the same for other libraries in Jupyter notebooks (e.g., numpy), so I am not sure if there is a way around this but will look into it more. If not, it motivates ordering the aliases in some way that reflects a likely usage order as much as possible.

Is it related to the Parameter.POSITIONAL_OR_KEYWORD setting?

@maxrjones
Copy link
Member Author

Is it related to the Parameter.POSITIONAL_OR_KEYWORD setting?

Yes, thanks. I went with the default setting of POSITIONAL_OR_KEYWORD, but we could support position independent tab completion by using KEYWORD_ONLY.

Actually, it might be worth considering setting other parameters to KEYWORD_ONLY. For example, I think all the parameters in pygmt.Figure.text are the default of POSITIONAL_OR_KEYWORD:

def text_(
    self,
    textfiles=None,
    x=None,
    y=None,
    position=None,
    text=None,
    angle=None,
    font=None,
    justify=None,
    **kwargs,
):

But KEYWORD_ONLY would be more appropriate for most because textfiles cannot be provided along with x and y so we do not really support positional arguments here:

def text_(
self,
textfiles=None,
*,
x=None,
y=None,
position=None,
text=None,
angle=None,
font=None,
justify=None,
**kwargs,
):

Or similarly only supporting keyword arguments for data onward for plot since x,y, anddata are not supported together:
def plot(self, x=None, y=None, *, data=None, size=None, direction=None, **kwargs):
Tab completion does seem to work for plot independent of parameter order, so I guess the only real benefit here is being more explicit about what is expected.

@maxrjones
Copy link
Member Author

One weakness to the current implementation is that all parameters defined by use_alias that are not already in the method signature are given a default value of None, but I think that required parameters (e.g, blockmean spacing and grdfill mode) should not have a default value.

@weiji14
Copy link
Member

weiji14 commented May 23, 2021

One weakness to the current implementation is that all parameters defined by use_alias that are not already in the method signature are given a default value of None, but I think that required parameters (e.g, blockmean spacing and grdfill mode) should not have a default value.

But any parameter with a value of None will disappear after being parsed by build_arg_str right? See also #639. So it should technically be ok to have a default None value?

@weiji14 weiji14 added the enhancement Improving an existing feature label May 23, 2021
@maxrjones
Copy link
Member Author

One weakness to the current implementation is that all parameters defined by use_alias that are not already in the method signature are given a default value of None, but I think that required parameters (e.g, blockmean spacing and grdfill mode) should not have a default value.

But any parameter with a value of None will disappear after being parsed by build_arg_str right? See also #639. So it should technically be ok to have a default None value?

You are correct. I had thought that required arguments should not have a default None value from a readability standpoint, in that parameter=None implies an optional argument. But I cannot find a source for that opinion.

@maxrjones maxrjones mentioned this pull request Jun 15, 2021
27 tasks
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
pygmt/helpers/__init__.py Outdated Show resolved Hide resolved
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks @meghanrjones. I've tested that this works in Jupyter (see below) but not in my Atom IDE as expected (but I can live with that). We can refine this in the future (post PyGMT v0.4.0) to fully resolve #1203, be it by reopening @seisman's proof of concept work at #1202 or otherwise.

image

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Jun 16, 2021
@seisman seisman added this to the 0.4.0 milestone Jun 16, 2021
@seisman seisman marked this pull request as ready for review June 16, 2021 02:50
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

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

That's a great improvement!

@weiji14 weiji14 changed the title Support tab completion by inserting aliases into the method signature Support tab completion in Jupyter by inserting aliases into the method signature Jun 16, 2021
@weiji14
Copy link
Member

weiji14 commented Jun 16, 2021

Made a small change to the title to clarify that tab completion only works on Jupyter (and IPython too, but that's implied). Feel free to squash and merge this in whenever @meghanrjones.

@maxrjones maxrjones merged commit d7b510f into master Jun 16, 2021
@maxrjones maxrjones deleted the inject-aliases branch June 16, 2021 22:53
@maxrjones maxrjones removed the final review call This PR requires final review and approval from a second reviewer label Jun 16, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…d signature (GenericMappingTools#1282)

Add a new insert_alias() function in pygmt/helpers/decorators.py that is used
by the use_alias() decorator to insert aliases into the method signature, to
support tab completion in Jupyter and iPython.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants