grdcontour Implementation #225

akshmakov · 2018-08-19T06:45:49Z

Implemented Figure.grdcontour that uses grdcontour gmt module

Plots grids to contours, with its own set of distinct inputs from pscontour

Test and Initial Documentation is complete

TODO

Expand Docstring for function.
Review chosen shorthand arguments for consistency with other modules
Refine tests
Regenerate baseline images.

stickler-ci · 2018-08-19T06:45:58Z

gmt/base_plotting.py

@@ -125,8 +125,51 @@ def coast(self, **kwargs):
        with Session() as lib:
            lib.call_module("coast", build_arg_string(kwargs))

+
+    @fmt_docstring


E303 too many blank lines (2)

stickler-ci · 2018-08-19T06:45:58Z

gmt/tests/test_grdcontour.py

+from ..exceptions import GMTInvalidInput
+from ..datasets import load_earth_relief
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-19T06:45:58Z

gmt/tests/test_grdcontour.py

+def test_grdcontour():
+    """Plot a contour image using an xarray grid
+    with fixed contour interval
+    """    


W291 trailing whitespace

stickler-ci · 2018-08-19T06:45:58Z

gmt/tests/test_grdcontour.py

+                   projection="W0/6i")
+    return fig
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-19T06:45:58Z

gmt/tests/test_grdcontour.py

+                   contour_interval="1000",
+                   annotation_interval="5000",
+                   projection="W0/6i",
+                   pen=["a1p,red","c0.5p,black"],


E231 missing whitespace after ','

stickler-ci · 2018-08-19T06:49:36Z

gmt/base_plotting.py

+                lib.call_module("grdcontour", arg_str)
+
+
+    @fmt_docstring


E303 too many blank lines (2)

akshmakov · 2018-08-19T07:16:15Z

@leouieda Build fails, due to test image mismatches, I suspect the density of features is playing a role, since 2 fail and 2 do not, and the ones that fail are denser contours. I will make a todo to refine the tests, but I suspect regenerating the images on the same machine (like what needed to be done for #212) will help. Not sure how to handle it

leouieda · 2018-08-20T20:31:19Z

@akshmakov I don't know why, but in #212 the image generated on a Mac fails the linux tests but the image I generated on my linux machine passes all tests. Let me know when you're done with the code and I'll regenerate the images on my machine.

leouieda · 2018-08-20T20:39:32Z

gmt/base_plotting.py

+        A="annotation_interval",
+        C="contour_interval",
+        B="frame",
+        G="label_placement",


Choosing these names is really hard. I would rename them to single words:

A="annotation"

C="interval" (contour is already in the method name so fig.grdcontour(contour_interval=..) seems a bit repetitive)

G="label"

I'm not sure about limit. This type of things is often called a range in other parts of GMT and some Python functions. But range is a builtin function so we should use ranges instead. I'm not a huge fan of that. Maybe we can leave limit for now until we can think of something better. This is one of those options that should actually be multiple options.

Please, feel free to disagree :) I'm not set on these names and am willing to change

You can tell my C bias as this was my attempt at less verbosity.

One thought I had is that -C can be aliased as cmap as per countour since it accepts a CPT file input as well, it just also accepts a number for contour intervals so is not compatible the other way. @mjziebarth maybe has some thoughts as well?

@leouieda One thing that happens is that something like contour_interval increases the verbosity when using the functions directly, but makes it clearer to construct keyword dictionaries. Application designers are more likely to use this pattern but it is still useful for managing plot options in scripts.

e.g.

OPTS={ projection:"W0/6i", coast:1, borders:1, annotation_interval:1000, contour_interval:100, } fig.coast(**OPTS) fig.grdcontour(grd, **OPTS)

vs

OPTS={ projection:"W0/6i", annotation:1000, interval:100, } # ...

More verbose names are easier to work with, and additionally reusing the keywords for incompatible parameters can prevent user from using a single dictionary to store common keyword arguments, but on the other hand this is already true in some cases and is certainly true for many GMT single letter arguments...

In any case I will take your advice for the names, and instead put my thoughts in #219 because I think this is a bigger discussion than grdcontour

From someone new to GMT: The more verbose names may make the difference between intuitively being able to write the code and having to consult the man pages.
But I agree with @akshmakov that this is probably best for general discussion e.g. in #219

Yeah, I agree. All of this will have to be updated at some point. For now, we need some time to use these functions and see where they fall short. No point in trying to predict all possible use cases.

@leouieda Updated with the following simplified keywords

A="annotation", B="frame", C="interval", G="label_placement", J="projection", L="limit", Q="cut", R="region", S="resample", U="logo", W="pen",

label_placement left as is to be consistent with Figure.contour

stickler-ci · 2018-08-23T06:53:47Z

gmt/helpers/decorators.py

+    separators = {"sequence": "/",
+                  "sequence_comma": ",",
+                  "sequence_plus": "+",
+    }


E124 closing bracket does not match visual indentation

stickler-ci · 2018-08-23T06:53:47Z

gmt/tests/test_grdcontour.py

+                   projection="W0/6i")
+    return fig
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-23T06:53:47Z

gmt/tests/test_grdcontour.py

+                   )
+    return fig
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-23T06:53:47Z

gmt/tests/test_grdcontour.py

+                   projection="M6i")
+    return fig
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-23T06:53:47Z

gmt/tests/test_grdcontour.py

+    )
+    return fig
+
+@pytest.mark.mpl_image_compare


E302 expected 2 blank lines, found 1

stickler-ci · 2018-08-23T06:53:49Z

gmt/tests/test_grdcontour.py

+    fig.grdcontour(
+                **comargs,
+                limit=(-25000,-1),
+                pen=["a1p,blue","c0.5p,blue"],


E231 missing whitespace after ','

stickler-ci · 2018-08-23T06:53:49Z

gmt/tests/test_grdcontour.py

+                **comargs,
+                limit=(-25000,-1),
+                pen=["a1p,blue","c0.5p,blue"],
+            )


E126 continuation line over-indented for hanging indent

stickler-ci · 2018-08-23T06:53:49Z

gmt/tests/test_grdcontour.py

+                pen=["a1p,blue","c0.5p,blue"],
+            )
+    fig.grdcontour(
+                **comargs,


E126 continuation line over-indented for hanging indent

stickler-ci · 2018-08-23T06:53:49Z

gmt/tests/test_grdcontour.py

+    fig.grdcontour(
+                **comargs,
+                limit="0",
+                pen=["a1p,black","c0.5p,black"],


E231 missing whitespace after ','

stickler-ci · 2018-08-23T06:53:49Z

gmt/tests/test_grdcontour.py

+                **comargs,
+                limit="0",
+                pen=["a1p,black","c0.5p,black"],
+            )


E126 continuation line over-indented for hanging indent

Plots grids to contours, with its own set of distinct inputs from `pscontour` Test and Initial Documentation is complete

stickler-ci · 2018-08-23T07:04:19Z

gmt/helpers/decorators.py

-    separators = {"sequence": "/", "sequence_comma": ","}
+    separators = {"sequence": "/",
+                  "sequence_comma": ",",
+                  "sequence_plus": "+",}


E231 missing whitespace after ','

stickler-ci · 2018-08-23T07:04:19Z

gmt/tests/test_grdcontour.py

+    """ Plot based on external contour level file """
+    fig = Figure()
+    comargs = {
+        'region': [-161.5,-154,18.5,23],


E231 missing whitespace after ','

stickler-ci · 2018-08-23T07:04:19Z

gmt/tests/test_grdcontour.py

+
+    fig.grdcontour(
+        **comargs,
+        limit=(-25000,-1),


E231 missing whitespace after ','

stickler-ci · 2018-08-23T07:04:19Z

gmt/tests/test_grdcontour.py

+        limit=(-25000,-1),
+        pen=["a1p,blue", "c0.5p,blue"],
+    )
+


W293 blank line contains whitespace

…ptions. Added additional kwarg list type that combines elements with + Added file contour definition for file based grdcontour test

akshmakov · 2018-08-23T07:14:18Z

@leouieda Finished expanded docs and keyword expansion. Removed WIP Status, ready to merge pending updated baseline plots

Are you on the island right now? Stay safe! 🌧️

P.s. Might baseline issue be due to default fonts? I am generating on linux machines as well and they fail on travis due to baseline mismatch, but pass on my machine

akshmakov · 2018-08-23T07:17:16Z

leouieda · 2018-09-18T00:03:11Z

@akshmakov thanks again for the contribution! I re-generated the baselines and fixed some code formatting issues. Once CI gods are happy, I'll merge this in!

leouieda · 2018-09-18T01:01:10Z

Thanks @akshmakov! Please feel free to add yourself to the AUTHORS.md file (through another PR).

akshmakov · 2018-09-21T04:44:53Z

@leouieda will do on another commit, hopefully for another issue, thank you for the baseline images, I'm happy to contribute!

Is there a style guide that I can review? Would black flag these instances? I only recently installed and learned to use it, so my future PR's shouldn't be annoying

P.S. I have some suspicion that baseline mismatches may be related to default system fonts.

leouieda · 2018-09-21T05:22:00Z

Is there a style guide that I can review?

Part of the appeal of using black is that it should handle this automatically. Still, I should clarify this in the contributing guide. In short, if your variable/function names are descriptive and make check is happy then everything should be fine :)

I have some suspicion that baseline mismatches may be related to default system fonts.

Yep, that seems like a very likely culprit. That's one of the issues with testing images but I don't think we can get around it easily.

Thanks for all your work on GMT and GMT/Python!

stickler-ci reviewed Aug 19, 2018

View reviewed changes

gmt/base_plotting.py Outdated

lib.call_module("grdcontour", arg_str)

@fmt_docstring

Copy link

stickler-ci Aug 19, 2018

Choose a reason for hiding this comment

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

E303 too many blank lines (2)

akshmakov force-pushed the pr/grdcontour-plotting-module branch from a84e5ab to 3292ba1 Compare August 19, 2018 06:50

leouieda reviewed Aug 20, 2018

View reviewed changes

stickler-ci reviewed Aug 23, 2018

View reviewed changes

akshmakov added 2 commits August 23, 2018 00:01

Implemented Figure.grdcontour that uses grdcontour gmt module

b76491f

Plots grids to contours, with its own set of distinct inputs from `pscontour` Test and Initial Documentation is complete

Stickler Whitespace Corrections

7d8f3e1

akshmakov force-pushed the pr/grdcontour-plotting-module branch from a21f2c3 to 1bc1686 Compare August 23, 2018 07:04

stickler-ci reviewed Aug 23, 2018

View reviewed changes

Updated Docstring for Function, added and refined tests, more alias o…

2d56e57

…ptions. Added additional kwarg list type that combines elements with + Added file contour definition for file based grdcontour test

akshmakov force-pushed the pr/grdcontour-plotting-module branch from 1bc1686 to 2d56e57 Compare August 23, 2018 07:07

akshmakov changed the title ~~WIP: grdcontour Implementation~~ grdcontour Implementation Aug 23, 2018

Rerender baselines and format code

4c4b471

leouieda merged commit 43badcd into GenericMappingTools:master Sep 18, 2018

weiji14 mentioned this pull request Dec 28, 2018

Wrap surface #245

Merged

5 tasks

grdcontour Implementation #225

grdcontour Implementation #225

Conversation

akshmakov commented Aug 19, 2018 • edited by leouieda Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshmakov commented Aug 19, 2018

leouieda commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshmakov Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshmakov commented Aug 23, 2018

akshmakov commented Aug 23, 2018

leouieda commented Sep 18, 2018

leouieda commented Sep 18, 2018

akshmakov commented Sep 21, 2018

leouieda commented Sep 21, 2018

akshmakov commented Aug 19, 2018 •

edited by leouieda

Loading

akshmakov Aug 23, 2018 •

edited

Loading