Skip to content

Conversation

@marqh
Copy link
Member

@marqh marqh commented Feb 28, 2014

new example for plotting ORCA data

@ajdawson
Copy link
Member

There are some PEP8 errors causing the tests to fail. Also you need to add a test for the example itself, this is pretty straightforward, just follow the lead of the other examples. A side benefit of adding a test is that we'll be able to see the resulting plot in the PR which makes it easier to review.

Copy link
Member

Choose a reason for hiding this comment

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

Typo: PLateCarree -> PlateCarree

Copy link
Member

Choose a reason for hiding this comment

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

Typo: coordinate -> coordinate

@pp-mo
Copy link
Member

pp-mo commented Feb 28, 2014

I've only really just started on this, but given tweaks to data source noted in the diff I have got it to run + I see the resulting plot is really rather big.

I'm not sure this plays nicely to the gallery
(see what I did there 😀 ?!? - I mean http://scitools.org.uk/iris/docs/latest/gallery.html)
Maybe more seriously, it doesn't display nicely in the rendered html example page, either.
This is like the problem I had with the log-anomaly-plot example in an early version

Maybe you should consider separate figures for this reason -- you really need to play with building the docs + checking the results in a browser, and even experiment a bit with different window sizes and zooms. Tiresome, but unavoidable IMHO.

@marqh
Copy link
Member Author

marqh commented Mar 3, 2014

these are now four separate figures

@ajdawson
Copy link
Member

ajdawson commented Mar 3, 2014

Looks like you've only got one output file... did you mean to put plt.show() inside the loop?

@marqh
Copy link
Member Author

marqh commented Mar 3, 2014

Looks like you've only got one output file... did you mean to put plt.show() inside the loop?

meant to | should have done: done now anyhow

cheers

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid variable names in all caps, it makes them look like constants. PC is a little cryptic too.

Copy link
Member

Choose a reason for hiding this comment

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

@marqh to ensure that the order of the tests run in the same order ...
for name in sorted(projections):

@marqh
Copy link
Member Author

marqh commented Mar 4, 2014

ok

Copy link
Member

Choose a reason for hiding this comment

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

@marqh You could slightly refactor this to be ...

    projections = dict(Mollweide=ccrs.Mollweide(),
                       PlateCarree=ccrs.PlateCarree(),
                       NorthPolarStereo=ccrs.NorthPolarStereo(),
                       Orthographic=ccrs.Orthographic(central_longitude=-90,
                                                      central_latitude=45))

@marqh
Copy link
Member Author

marqh commented Mar 6, 2014

@bjlittle
I'm afraid I don't see how this refactor makes the code any better/worse

@bjlittle bjlittle self-assigned this Mar 7, 2014
@bjlittle
Copy link
Member

bjlittle commented Mar 7, 2014

Lovely example, thanks @marqh 👍

bjlittle added a commit that referenced this pull request Mar 7, 2014
@bjlittle bjlittle merged commit e27db47 into SciTools:master Mar 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants