Skip to content
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

CF CoordSystems (replacement pull) #65

Merged
merged 5 commits into from
Sep 11, 2012
Merged

Conversation

bblay
Copy link
Contributor

@bblay bblay commented Sep 3, 2012

This pull replaces #52, which had problems with the commit history.

This was referenced Sep 3, 2012
@ghost ghost assigned rhattersley Sep 5, 2012
@bblay
Copy link
Contributor Author

bblay commented Sep 5, 2012

Can't see the snippets for this pull request, can only see the code in the diffs view.

@bblay
Copy link
Contributor Author

bblay commented Sep 5, 2012

Outstanding issues:

@bblay
Copy link
Contributor Author

bblay commented Sep 5, 2012

why can't this be merged automatically?
I just merged with master and it didn't help.

@rhattersley
Copy link
Member

Don't merge from master! Rebase instead.

@bblay bblay mentioned this pull request Sep 7, 2012
@bblay bblay closed this Sep 7, 2012
@bblay bblay reopened this Sep 7, 2012
@bblay
Copy link
Contributor Author

bblay commented Sep 7, 2012

oops

raise ValueError("The source and grid cubes must contain a HorizontalCS.")
source_cs = source_cube.coord_system(iris.coord_systems.CoordSystem)
grid_cs = grid_cube.coord_system(iris.coord_systems.CoordSystem)
if source_cs==None != grid_cs==None:
Copy link
Member

Choose a reason for hiding this comment

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

Both occurrences of == should be is.

@bblay
Copy link
Contributor Author

bblay commented Sep 10, 2012

Since #73 has been now raised I believe the only outstanding issue is the curl tests, still under discussion.
(I'm happy to do #73 in this branch, if preferred)

@rhattersley
Copy link
Member

Let's leave #73 as a separate thread - I don't want to have to review all these changes again! ;-)

So that just leaves curl to be discussed with @pelson.

@bblay
Copy link
Contributor Author

bblay commented Sep 11, 2012

It seems it's just the comments that are incorrect here and here
Can I suggest it's dealt with in another ticket?

In fact, I'm not now convinced they are incorrect. They do seem to use r in the definition of F.

@rhattersley
Copy link
Member

OK - let's separate out the curl issue. It still looks like it's broken to me so I'd like someone else (e.g. @pelson) to take a look. Increasing the radius should decrease the curl, and this should be countered by the increase in F - i.e. there should be no net change.

rhattersley added a commit that referenced this pull request Sep 11, 2012
CF-style CoordSystem classes
@rhattersley rhattersley merged commit 59a82e9 into SciTools:master Sep 11, 2012
This was referenced Sep 11, 2012
@bblay
Copy link
Contributor Author

bblay commented Sep 11, 2012

@rhattersley , please also note the units are 1000 times smaller, here and here

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