Skip to content

Conversation

@bblay
Copy link
Contributor

@bblay bblay commented Aug 22, 2012

No description provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where #52 is needed. The code is commented out, ready to use after #52 has been brought in.

@esc24
Copy link
Member

esc24 commented Aug 23, 2012

@bblay I've taken a look at nimrod.py and put a bunch of comments inline. I've also looked over your proposed changes to the file format mechanism. I have a number of issues that I'd like to discuss on this, and I'd rather not mix up the two discussions or hold up the NIMROD functionality any more than necessary. Could you split these two sets of changes out into two separate pull requests?

@bblay
Copy link
Contributor Author

bblay commented Aug 29, 2012

All review avctions implemented except:

Format recognition moved into https://github.com/bblay/iris/tree/formats for later.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this behaviour. Have you come across such units?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the sample data I'm loading is in m/2-25k !

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little uneasy about this too, but improving the unit handling is a significant chunk of work. Although this differs from the rest of the loaders (which don't attempt to catch the exception) I think it's reasonable workaround. I think a warning is in order though to let the user know the units could not be parsed and have been squirrelled away.

@pelson
Copy link
Member

pelson commented Aug 31, 2012

On the whole, I think the nimrod reading would benefit from using numpy.fromfile rather than struct. Other than that, I think its a good start into reading Nimrod.

@pelson
Copy link
Member

pelson commented Aug 31, 2012

I should add that it would be nice to extract the names of the nimrod variables out to a tuple so that should anyone wish to implement saving in the future, the process would be fairly straight forward.

Copy link
Member

Choose a reason for hiding this comment

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

Should there be a double ':' here?

@bblay bblay mentioned this pull request Sep 12, 2012
@esc24
Copy link
Member

esc24 commented Sep 12, 2012

@bblay has created issue #84 to tackle plotting of cubes loaded from NIMROD. This breaks some of the dependencies so we can get this PR merged.

@bblay
Copy link
Contributor Author

bblay commented Sep 12, 2012

Ready.

Copy link
Member

Choose a reason for hiding this comment

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

long line

@bblay
Copy link
Contributor Author

bblay commented Sep 12, 2012

i do believe it is ok to go.
shall i squash first?

Copy link
Member

Choose a reason for hiding this comment

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

Don't need the ~.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ~ make it say Cube rather than iris.cube.Cube.
Will keep if that's ok.

Copy link
Member

Choose a reason for hiding this comment

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

Until we can agree on a standard for this I'm going to go with the author's preference. Easy fix if we change our minds.

@bblay
Copy link
Contributor Author

bblay commented Sep 12, 2012

With hindsight, it's probably not helpful for me to be continually doing review actions and pushing rebased commits while you're still going. Probably better to wait until reviewers signal they have finished.

esc24 added a commit that referenced this pull request Sep 12, 2012
@esc24 esc24 merged commit 36dce22 into SciTools:master Sep 12, 2012
@esc24 esc24 mentioned this pull request Sep 12, 2012
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