Skip to content

Conversation

ajdawson
Copy link
Member

Adds 1D scatter plot functionality to iris.plot and iris.quickplot, capitalising on the new 1D plotting features added in #593.

Some of the test plots are not too pretty, but they should serve well enough!

Copy link
Member

Choose a reason for hiding this comment

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

Did you intend this to have the c=c argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yes I think I did. Not that it matters much though. I'll sort it out.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed - it doesn't matter much at all but it gives the previous line a reason to exist.

FWIW I wasn't doing a full review, I just happened to spot it when I was looking for some code which used the new function because I was pondering the overlap between iplt.points and iplt.scatter.

@rhattersley
Copy link
Member

Neat 😀

@ajdawson
Copy link
Member Author

The omission of the c=c keywords in two of the tests highlighted by @rhattersley have been corrected.

@rhattersley
Copy link
Member

It looks like it'd make sense for the quickplot version to add a colorbar, as for contourf and pcolor[mesh]?

@ajdawson
Copy link
Member Author

It looks like it'd make sense for the quickplot version to add a colorbar, as for contourf and pcolor[mesh]?

I guess it might do, I hadn't thought of that. It would only make sense to do this when the c= keyword argument is given. Since this argument is not a cube or a coordinate (currently) the colorbar would not have a label with units as they would be unknown.

@rhattersley
Copy link
Member

It would only make sense to do this when the c= keyword argument is given.

Good point. I'd be happy to leave this for another time - perhaps adding cube/coord support to the c argument at the same time.

@ajdawson
Copy link
Member Author

Allowing c to be a cube or a coordinate is somewhat non-trivial. Possible sure, but not as straightforward as one might think initially. It raises other questions such as: is c *required` to be a cube or a coordinate?

Anyway, if you are happy for colorbars to be left out then I'm happy to leave them out of this PR.

@rhattersley
Copy link
Member

Anyway, if you are happy for colorbars to be left out then I'm happy to leave them out of this PR.

👍

@rhattersley
Copy link
Member

Please could you rebase 7f789386a7bc19a158a93fc721fa253590f7709d onto the latest master (which includes #634) so the Travis tests can run.

@ajdawson
Copy link
Member Author

Rebased and tests passing.

@rhattersley
Copy link
Member

Lovely. Thanks @ajdawson 😄

rhattersley added a commit that referenced this pull request Jul 24, 2013
NF - add 1D scatter plot functions.
@rhattersley rhattersley merged commit 1e92577 into SciTools:master Jul 24, 2013
@ajdawson ajdawson deleted the scatter-1d branch July 24, 2013 16:09
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.

2 participants