Split meca into a standalone module from base_plotting.py #686

weiji14 · 2020-11-07T10:05:00Z

Description of proposed changes

Begin to split up base_plotting.py into standalone components. Starting with the largest module meca.

New Structure:

pygmt/
  ├── src/ (new folder)
  |        └── meca.py (contains `def meca()` function)
  └── base_plotting.py (contains a line `from pygmt.src.meca import meca`)

The following modules from base_plotting.py may be split up in separate PRs:

coast
colorbar
grdcontour
grdimage
grdview
plot
plot3d
contour
basemap
logo
image
legend
text

References:

Addresses #685

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.

Notes

You can write /format in the first line of a comment to lint the code automatically

Also fixed some relative imports.

willschlitzer · 2021-01-24T20:55:17Z

@weiji14 I'm glad you figured out a way to do this! I'm not trying to simplify the work you have done on this, but is this as "simple" as moving a single function to it's own script and adding "from pygmt.src.script import script function? As far as I can tell, that is the case, but I want to check with you before doing work/submitting pull requests for the other functions, especially those that I'm in the process of wrapping.

Also, are we trying to get this completed for PyGMT v0.3? If so, do we have an expected deadline for that? I'm assuming it's before the GMT 6.2 release.

weiji14

Ok, I'm opening this up for review since things kinda work, but need some critical comments on this.

Also, are we trying to get this completed for PyGMT v0.3? If so, do we have an expected deadline for that? I'm assuming it's before the GMT 6.2 release.

Yes, best to release PyGMT v0.3.0 before GMT 6.2 (scheduled for mid-Feb, but traditionally it's always been late). It'll be good to cut a release once we have a good number of new 'features' like inset and such.

weiji14 · 2021-01-24T21:01:17Z

pygmt/base_plotting.py

-                arg_str = " ".join([fname, build_arg_string(kwargs)])
-                lib.call_module("meca", arg_str)
+    # GMT Supplementary modules
+    from pygmt.src.meca import meca  # pylint: disable=import-outside-toplevel


is this as "simple" as moving a single function to it's own script and adding "from pygmt.src.script import script function? As far as I can tell, that is the case, but I want to check with you before doing work/submitting pull requests for the other functions, especially those that I'm in the process of wrapping.

Yes, this is the key line. It looks simple and works, but:

we would need to write a separate line for each new module (e.g. velo, solar, etc).

I don't really like having the pylint disable=import-outside-toplevel here.

To be honest, there's probably a better way to do this. Any good ideas? Or we can just settle for this for now.

Is it possible to have from pygmt.src.meca import meca in pygmt/src/__init__.py, then we only need from pygmt.src import meca, velo, text here.

Yep sure, I'll do that, but it would require changing two files every time we wrap a new module (instead of one right now).

weiji14 · 2021-01-24T21:01:51Z

pygmt/src/meca.py

+    """
+


Note that fig.meca doesn't appear to use self._preprocess like the other fig modules, but probably doesn't matter according to #685 (comment)?

Seems OK to me.

pygmt/src/meca.py

seisman · 2021-01-24T21:34:55Z

The final goal is to have one file for one module, including both plotting and non-plotting modules, right? If so, is it better to put these files in a modules directory (I know we already have a modules.py file)?

weiji14 · 2021-01-24T21:45:42Z

The final goal is to have one file for one module, including both plotting and non-plotting modules, right?

Not necessarily? We'll need to discuss this more in #685 (or a separate issue), it makes sense for the plotting stuff because there's so many.

If so, is it better to put these files in a modules directory (I know we already have a modules.py file)?

I chose src because GMT and GMT.jl uses src, and so does matplotlib. PyGMT originally had a modules folder in the early days as I mentioned in #685 (comment), but yeah, it's not good having a modules.py and modules folder. Happy to change it to another name though.

Co-Authored-By: Dongdong Tian <[email protected]>

seisman · 2021-01-26T06:15:01Z

This PR looks good to me, but I hesitate to merge it into v0.3.0 release. Do we have enough time and human resources to finish all the modules (plotting, and maybe also non-plotting modules) before v0.3.0?

weiji14 · 2021-01-26T07:10:32Z

From a user point of view, nothing actually changes. This PR won't close #685, but it will at least enable others to start putting new functions under the src folder instead of base_plotting.py. The other functions (e.g. grdimage, grdview, etc can be moved after v0.3.0) and I should have more time next month once I hand in my thesis to review our growing backlog.

…pingTools#686) Begin to split up base_plotting.py into standalone components. Starting with the largest module meca. * Split meca out from base_plotting.py under a pygmt/src folder * Reduce diff and fix pylint/import errors * Silence import-outside-toplevel error * Update __init__.py with meca import * Re-wrap docstrings to fit 79 characters Co-authored-by: Dongdong Tian <[email protected]>

weiji14 added the maintenance Boring but important stuff for the core devs label Nov 7, 2020

vercel bot temporarily deployed to Preview November 7, 2020 21:58 Inactive

weiji14 changed the title ~~Split up base_plotting.py into standalone modules~~ Split meca into a standalone module from base_plotting.py Nov 7, 2020

weiji14 mentioned this pull request Dec 2, 2020

Time to split up base_plotting.py into individual module files #685

Closed

weiji14 mentioned this pull request Dec 20, 2020

Convert relative imports to absolute imports #754

Merged

5 tasks

weiji14 added 3 commits January 25, 2021 09:03

Move figure.py and base_plotting.py under a pygmt/figure folder

84853c9

Also fixed some relative imports.

Split meca out from base_plotting.py

6220a6b

Keep figure.py and base_plotting.py in original directory

bc64ec2

weiji14 force-pushed the restructure_base_plotting branch from 613ae9b to bc64ec2 Compare January 24, 2021 20:09

vercel bot temporarily deployed to Preview January 24, 2021 20:09 Inactive

Reduce diff and fix pylint/import errors

392d0b5

vercel bot temporarily deployed to Preview January 24, 2021 20:30 Inactive

Remove unused argument self and silence import-outside-toplevel error

8444f21

vercel bot temporarily deployed to Preview January 24, 2021 20:47 Inactive

Merge branch 'master' into restructure_base_plotting

a555868

vercel bot temporarily deployed to Preview January 24, 2021 20:53 Inactive

weiji14 commented Jan 24, 2021

View reviewed changes

weiji14 marked this pull request as ready for review January 24, 2021 21:12

Actually still need to use self somehow

89b62a2

vercel bot temporarily deployed to Preview January 24, 2021 21:23 Inactive

vercel bot temporarily deployed to Preview January 24, 2021 21:45 Inactive

weiji14 force-pushed the restructure_base_plotting branch from 8893877 to ef2d002 Compare January 24, 2021 21:47

Update __init__.py with meca import

f8a00ec

Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 force-pushed the restructure_base_plotting branch from ef2d002 to f8a00ec Compare January 24, 2021 21:49

vercel bot temporarily deployed to Preview January 24, 2021 21:52 Inactive

Re-wrap docstrings to fit 79 characters

5fd67b9

vercel bot temporarily deployed to Preview January 24, 2021 21:58 Inactive

weiji14 added this to the 0.3.0 milestone Jan 24, 2021

Merge branch 'master' into restructure_base_plotting

bd66e3b

vercel bot temporarily deployed to Preview January 26, 2021 17:59 Inactive

seisman approved these changes Jan 26, 2021

View reviewed changes

weiji14 merged commit 2345486 into master Jan 26, 2021

weiji14 deleted the restructure_base_plotting branch January 26, 2021 21:13

willschlitzer mentioned this pull request Jan 28, 2021

Move plotting functions to separate modules #808

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split meca into a standalone module from base_plotting.py #686

Split meca into a standalone module from base_plotting.py #686

weiji14 commented Nov 7, 2020 •

edited

Loading

willschlitzer commented Jan 24, 2021 •

edited

Loading

weiji14 left a comment •

edited

Loading

weiji14 Jan 24, 2021

seisman Jan 24, 2021

weiji14 Jan 24, 2021

weiji14 Jan 24, 2021

seisman Jan 26, 2021

seisman commented Jan 24, 2021 •

edited

Loading

weiji14 commented Jan 24, 2021

seisman commented Jan 26, 2021

weiji14 commented Jan 26, 2021 •

edited

Loading

Split meca into a standalone module from base_plotting.py #686

Split meca into a standalone module from base_plotting.py #686

Conversation

weiji14 commented Nov 7, 2020 • edited Loading

willschlitzer commented Jan 24, 2021 • edited Loading

weiji14 left a comment • edited Loading

Choose a reason for hiding this comment

weiji14 Jan 24, 2021

Choose a reason for hiding this comment

seisman Jan 24, 2021

Choose a reason for hiding this comment

weiji14 Jan 24, 2021

Choose a reason for hiding this comment

weiji14 Jan 24, 2021

Choose a reason for hiding this comment

seisman Jan 26, 2021

Choose a reason for hiding this comment

seisman commented Jan 24, 2021 • edited Loading

weiji14 commented Jan 24, 2021

seisman commented Jan 26, 2021

weiji14 commented Jan 26, 2021 • edited Loading

weiji14 commented Nov 7, 2020 •

edited

Loading

willschlitzer commented Jan 24, 2021 •

edited

Loading

weiji14 left a comment •

edited

Loading

seisman commented Jan 24, 2021 •

edited

Loading

weiji14 commented Jan 26, 2021 •

edited

Loading