-
Notifications
You must be signed in to change notification settings - Fork 365
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
Add _repr_html_ to Projection class #951
Conversation
@dkillick We seem to be having some shapereader issues here. I've seen this before in the last few days but I can't remember what the problem was. Has the testing version changed or anything? |
Needs a rebase. |
5f34853
to
9ac41b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkillick I forgot, I've already had a look and it looks good to me. I'm happy if you are.
That doesn't really matter of course, what matters is that @pp-mo is happy because he's got the big green button...
lib/cartopy/crs.py
Outdated
@@ -146,6 +146,13 @@ def domain(self): | |||
domain = self._domain = sgeom.Polygon(self.boundary) | |||
return domain | |||
|
|||
def _repr_html_(self): | |||
import matplotlib.pyplot as plt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment to explain context.
? something like ? "enables self-display in a Jupyter notebook"
Other than missing explanatory comment, |
I don't think this is correct. You are a) not actually returning any HTML for the |
I have to agree here. This seems like an abuse of the |
Apologies I've clearly missed the point here. What was the scope of this intended to be, then ? |
|
@ajdawson @QuLogic I hope you're both satisfied, it's taken all afternoon to get this returning html.
Of course the end result is worthwhile. You can now see what the Projection looks like, which is the most obviously useful way to represent a cartopy Projection ever, especially when we're operating in such an always-visual environment as a jupyter notebook. |
Not yet I'm afraid. The problem as I see it is that a repr of any kind shouldn't interfere with the state of a program. Your implementation modifies the state of pyplot by overwriting the current figure. To illustrate, let's say I'm working in a notebook with interactive plotting, I make a plot of something I'm interested in. I then construct a crs and look at its repr as in your example, now my figure is gone, replaced with the repr. This functionality really needs to be implemented in a way that is side-effect free, because users should not have to care what state their notebook is in before they use it.
I'm not disputing that it isn't a useful way to see what a projection looks like. I'm suggesting that the implicit and potentially destructive modification of pyplot state is not worth it over the alternative which is simply: crs = ccrs.Stereographic()
ax = plt.axes(projection=crs)
ax.coastlines() This is both totally explicit (user knows what is going to happen) and is only a small amount of extra typing. If you want to push on and implement this feature in a side-effect free way then that is cool by me, but you'll have to consider if it is worth spending the extra time. |
3f8dbfe
to
234ba58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work fine with a small tweak to creating the Matplotlib side of things.
But on a side note, isn't there a _repr_png_
as well that could be used to avoid needing to wrap with <img>
?
lib/cartopy/crs.py
Outdated
from io import BytesIO | ||
import matplotlib.pyplot as plt | ||
# Produce a visual repr of the Projection instance. | ||
ax = plt.axes(projection=self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fig, ax = plt.subplots(subplot_kw={'projection': self})
to avoid working on the current figure.
Given that we’re talking pretty detailed coastlines and the raster image is mostly white, I’d not expect SVG to be much, if any, smaller. I can live with either option. |
Yes to doing this kind of thing. It is a hugely beneficial change. This ties in closely with some work on improving the rendering of the projection list (which currently has some ugly rendering, particularly of the UTC projection). It like the projections used in the list to be the same as the projection used in the repr. The relevant doc code is currently at https://github.com/SciTools/cartopy/blob/master/docs/make_projection.py#L93. Happy to take a look at bringing the two things together, but just wanted to share my hopes for what the |
lib/cartopy/crs.py
Outdated
# "Save" to a bytestring. | ||
fmt = 'png' | ||
buf = BytesIO() | ||
plt.savefig(buf, format=fmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the comment above, can use fig.savefig()
and fig.close()
Any chance you can get this cleaned up and rebased in time for Friday’s release? |
234ba58
to
61e3173
Compare
@dopplershift thanks for the poke! This is now rebased and all good suchlike. I've also moved to |
# "Rewind" the buffer to the start and return it as an svg string. | ||
buf.seek(0) | ||
return buf.read() | ||
finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take out the finally
clause? Because it’s in the finally
, that code runs for every invocation of _repr_svg
. Also, just calling repr
wouldn’t be enough, you need to return it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, returning a string as an SVG isn't going to end well I don't think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to include the repr(projection)
in there too. We can do this pretty easily with a _repr_html_
(sorry for flip-flopping since my comment last year!).
fig, ax = plt.subplots(figsize=(5, 3), | ||
subplot_kw={'projection': self}) | ||
ax.set_global() | ||
ax.coastlines('110m') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try out the 'auto' scale... might produce a more desirable result for limited area maps.
# "Rewind" the buffer to the start and return it as an svg string. | ||
buf.seek(0) | ||
return buf.read() | ||
finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, returning a string as an SVG isn't going to end well I don't think.
pass | ||
else: | ||
if six.PY2: | ||
from StringIO import StringIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just use six here. six.StringIO
- might actually make migration to a python 3 only codebase easier too.
I tried pushing to this PR, but alas don't have the foo. Instead, here is my proposed
Result: |
That doesn't seem necessary. If you return |
But having a def foo():
try:
return 5
finally:
return 6
print(foo()) prints |
Yes, drop the |
Docs are hard to find, but there is this comment:
so |
Confirmed that you are both spot on. Have implemented in #1196. |
Awesome! Great stuff @dkillick. Thanks for persevering! |
Provides a visual repr (!) of any cartopy
Projection
subclass in jupyter notebook.For example:
Also works interactively: