Added basic contour functionality #212

mjziebarth · 2018-07-12T15:18:59Z

Hi everyone,
started using gmt-python yesterday and really like the project. Since I'll be writing some solutions for my own use, I figured I may contribute them right away. So, I am leaning a bit on the pull-request-style of development and don't expect this to be ready to merge straight away.

Changes proposed in this pull request

Add function to call gmt contour from BasePlotting.

Note

The code is copied from the plot function and is very basic so far. Tested only locally for crashes in Figure so far.

Looking forward to your input!

stickler-ci · 2018-07-12T15:19:55Z

gmt/base_plotting.py

+        kwargs = self._preprocess(**kwargs)
+
+        kind = data_kind(data, x, y)
+


W293 blank line contains whitespace

leouieda · 2018-07-13T20:07:48Z

@mjziebarth thank you submitting this! I really appreciate it 👍

Don't worry too much about the whole flow of development, I'll try to help you through it. And there is no way to really break things. That's what all these automatic checks take care of.

I'm a little bit overwhelmed with the Scipy conference this week, so sorry for the late reply. I'll try to look over this on the weekend.

Thank you again for your contribution! It's really appreciated.

leouieda · 2018-07-13T21:33:51Z

@mjziebarth the code looks ok to me. Are you getting crashes?

leouieda · 2018-07-13T21:37:14Z

If not, then the only things we need are a bit more documentation and some tests.

For the tests, please have a look at this: https://github.com/GenericMappingTools/gmt-python/blob/master/CONTRIBUTING.md#testing-plots Let me know if you need any help.

For the documentation, I'm thinking of adding a few examples to all modules. Would be willing to try to add an example plot using our sphinx extension?

mjziebarth · 2018-07-16T18:49:10Z

Thanks for the feedback!

I added some test cases. Not the most sophisticated but should get the job done.
The tests ultimately fail on my local machine (TypeError with the gmt.Figure) but the images were generated. Let's see how this works with the CI.

I could add a test case for sphinx, but that should probably be a little bit more real-world, I guess?

stickler-ci · 2018-07-16T18:59:35Z

gmt/tests/test_contour.py

+    "Contour should raise an exception if no or not sufficient data "
+    "is given"
+    fig = Figure()
+    for x in [None,data[:,0]]:


E231 missing whitespace after ','

stickler-ci · 2018-07-16T18:59:36Z

gmt/tests/test_contour.py

+    "is given"
+    fig = Figure()
+    for x in [None,data[:,0]]:
+        for y in [None,data[:,0]]:


E231 missing whitespace after ','

stickler-ci · 2018-07-16T18:59:36Z

gmt/tests/test_contour.py

+    fig = Figure()
+    for x in [None,data[:,0]]:
+        for y in [None,data[:,0]]:
+            for z in [None,data[:,0]]:


E231 missing whitespace after ','

stickler-ci · 2018-07-16T18:59:36Z

gmt/tests/test_contour.py

+                if x is not None and y is not None and z is not None:
+                    continue
+                with pytest.raises(GMTInvalidInput):
+                     fig.contour(


E111 indentation is not a multiple of four

stickler-ci · 2018-07-16T18:59:36Z

gmt/tests/test_contour.py

+def test_contour_vec(region):
+    "Plot an x-centered gaussian kernel with different y scale"
+    fig = Figure()
+    x,y = np.meshgrid(np.linspace(region[0],region[1]),


E231 missing whitespace after ','

stickler-ci · 2018-07-16T18:59:36Z

gmt/tests/test_contour.py

+    x = x.flatten()
+    y = y.flatten()
+    z = (x-0.5*(region[0]+region[1]))**2 + 4*y**2
+    z = np.exp(-z/10**2*np.log(2))


E226 missing whitespace around arithmetic operator

stickler-ci · 2018-07-16T18:59:37Z

gmt/tests/test_contour.py

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


E302 expected 2 blank lines, found 1

stickler-ci · 2018-07-16T18:59:37Z

gmt/tests/test_contour.py

+    return fig
+
+@pytest.mark.mpl_image_compare
+def test_contour_matrix(data,region):


E231 missing whitespace after ','

stickler-ci · 2018-07-16T18:59:37Z

gmt/tests/test_contour.py

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


E302 expected 2 blank lines, found 1

stickler-ci · 2018-07-16T18:59:37Z

gmt/tests/test_contour.py

+        pen='#ffcb87'
+    )
+    return fig
+


W391 blank line at end of file

…to contour

mjziebarth · 2018-07-17T11:05:12Z

So right now there are errors left in some of the Travis runs. One is a one pixel width difference in the generated test images that prevents the comparison. From visual inspection, the images do not show obvious errors and it seems like the python 3.6 Travis runs without errors.

Another Travis run fails at some point before that.

Do you have any idea what could go wrong there or how that behavior could be fixed? I'm not familiar with the underlying testing framework.

Regarding the sphinx-example: Where would that go?

leouieda · 2018-07-24T00:50:18Z

@mjziebarth I'm going through the CI logs to try to see what went wrong. I found a problem with our CI builds on 3.5. It seems that our code formatter black only works on 3.6. I'll fix this first so that your tests can pass.

akshmakov · 2018-08-17T06:53:00Z

I had a need for contours as well, started working on this and then saw it was already done! Nice.

I can help rebase on top of the existing master branch so the PR can go through, but am waiting for this functionality!

(if you don't have any merge conflicts as easy as git rebase master or my preference git rebase -i master to squash and re-word commits)

+1

akshmakov · 2018-08-17T07:02:49Z

The branch rebases cleanly on master, meaning a merge commit from master isn't required. For an up to date branch as an example https://github.com/nc5ng/gmt-python/tree/rebase/contour-pr-212

@leouieda Turns out that grdcontour that I was looking for is different than pscontour, would it be worth adding my grdcontour implementation to this PR or creating a new PR?

mjziebarth · 2018-08-17T11:08:44Z

Okay, I did it using this button of github. Not sure what that actually does but hope that's okay!

@leouieda Please excuse if you've been busy, but is there anything else I should do in this PR?

leouieda · 2018-08-17T21:46:10Z

@mjziebarth sorry for the absence. I was away on travel last week and spent this entire week trying to get things up and running again with the GMT conda package (so many headaches). We're up and running again now.

Only thing left to do here is list the Figure.contour method in doc/api/index.rst. After that, I'll merge as soon as the CIs builds are passing.

Thanks for your effort and your patience! I hope to see more of these 😃

leouieda · 2018-08-17T21:46:57Z

@akshmakov yeah, grdcontour is a different module with different inputs. Would you mind adding it in another PR?

They were failing everything except python 3.6 on OSX for some reason.

leouieda · 2018-08-17T22:07:56Z

@mjziebarth there was a strange problem with your baseline images and I'm not entirely sure what caused it. I regenerated them on my machine and went ahead and added the API entry so we can merge this.

welcome · 2018-08-17T23:06:44Z

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

Added basic contour functionality

fcc4864

stickler-ci reviewed Jul 12, 2018

View reviewed changes

gmt/base_plotting.py Outdated

kwargs = self._preprocess(**kwargs)

kind = data_kind(data, x, y)

Copy link

stickler-ci Jul 12, 2018

Choose a reason for hiding this comment

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

W293 blank line contains whitespace

Remove whitespace in empty line

4091880

Added test cases for contour.

b73786b

Actually added the contour test code.

ce4eee2

stickler-ci reviewed Jul 16, 2018

View reviewed changes

mjziebarth and others added 6 commits July 16, 2018 21:12

Code style changes

e8bc52c

Style changes 2

63f3cd1

Moved baseline images to right directory.

f03a64c

Different test setup

5e84d3f

Merge branch 'contour' of https://github.com/mjziebarth/gmt-python in…

0e0d9c1

…to contour

Reduce contour test complexity

aa2ae41

leouieda mentioned this pull request Jul 24, 2018

Check that Python is not being upgraded fatiando/continuous-integration#4

Merged

Merge branch 'master' into contour

58fcade

leouieda mentioned this pull request Jul 24, 2018

Don't install black on TravisCI automatically #216

Closed

Merge branch 'master' into contour

8a9641e

Update baseline figures for contour

8d7095a

They were failing everything except python 3.6 on OSX for some reason.

Add contour entry to the API index

d49d5d6

Fix formatting errors

bb7c0d2

leouieda merged commit e5845ba into GenericMappingTools:master Aug 17, 2018

akshmakov mentioned this pull request Aug 19, 2018

grdcontour Implementation #225

Merged

4 tasks

		kwargs = self._preprocess(**kwargs)

		kind = data_kind(data, x, y)

Added basic contour functionality #212

Added basic contour functionality #212

Uh oh!

Conversation

mjziebarth commented Jul 12, 2018

Changes proposed in this pull request

Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leouieda commented Jul 13, 2018

Uh oh!

leouieda commented Jul 13, 2018

Uh oh!

leouieda commented Jul 13, 2018

Uh oh!

mjziebarth commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjziebarth commented Jul 17, 2018

Uh oh!

leouieda commented Jul 24, 2018

Uh oh!

akshmakov commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akshmakov commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjziebarth commented Aug 17, 2018

Uh oh!

leouieda commented Aug 17, 2018

Uh oh!

leouieda commented Aug 17, 2018

Uh oh!

leouieda commented Aug 17, 2018

Uh oh!

welcome bot commented Aug 17, 2018

Uh oh!

Uh oh!

akshmakov commented Aug 17, 2018 •

edited

Loading

akshmakov commented Aug 17, 2018 •

edited

Loading