Skip to content

Exposecm #2039

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

Merged
merged 3 commits into from
Jun 3, 2016
Merged

Exposecm #2039

merged 3 commits into from
Jun 3, 2016

Conversation

marqh
Copy link
Member

@marqh marqh commented Jun 3, 2016

a simple rebase of
#1981

@DPeterK
Copy link
Member

DPeterK commented Jun 3, 2016

Note the fact that this is just a rebased version of #1981 means that it retains that PR's unanswered question relating to cf_var_name. Options:

  • we merge this as it is, which has the advantage of being nice and quick but the disadvantage of potentially introducing lower-quality code into Iris. That said, apparently cell methods parsing has worked since it was introduced, so it can't be that bad...
  • we wait for the extra effort to get to the bottom of the issue, decide whether it is an issue, make the relevant changes, test them, remove/edit other tests, and then try to get this merged. Oh, and we potentially need to wait for the xarray guys (see comment to this effect). Benefits... higher quality code; downsides... missing the 1.10 milestone.

Considering these, I'm in favour of the former - we can add missing stuff later. @pelson @bjlittle thoughts? You both have prior on these changes.

@DPeterK DPeterK added this to the v1.10 milestone Jun 3, 2016
@DPeterK
Copy link
Member

DPeterK commented Jun 3, 2016

I guess this should also have a whatsnew entry.

@marqh
Copy link
Member Author

marqh commented Jun 3, 2016

I guess this should also have a whatsnew entry.

added

+1 for

merge this as it is

@DPeterK
Copy link
Member

DPeterK commented Jun 3, 2016

Good stuff @marqh, I'm merging. Follow-up work can be done as a follow-up once it's better understood what, if anything, needs to be done about cf_var_name.

@DPeterK DPeterK merged commit 3bc477d into SciTools:master Jun 3, 2016
@DPeterK
Copy link
Member

DPeterK commented Jun 3, 2016

Thanks to @rhattersley (for originally doing the work) and @marqh (for updating) 🎉

@pp-mo
Copy link
Member

pp-mo commented Jun 3, 2016

we merge this as it is,...
apparently cell methods parsing has worked since it was introduced, so it can't be that bad..
we wait ... decide whether it is an issue

Re-reading, I don't think there is any outstanding functional problem, and I wasn't proposing we should enhance the routine : I'd like it to be a bit clearer about what it can and can't handle, and cover that in the documentation.

I don't think the xarray related issue makes any difference at this point.

Ideally, we would remove the functionally superfluous 'cf_name' arg.
As a minimum, just enhance the documentation + move on.

you merged it ?!!

@DPeterK
Copy link
Member

DPeterK commented Jun 3, 2016

I'm in favour of the former - we can add missing stuff later

you merged it?!!

Yes. Feel free to add anything you feel is missing; I didn't feel anything was missing that was dramatic enough to add yet further drag to the 1.10 release .

@pp-mo
Copy link
Member

pp-mo commented Jun 3, 2016

Feel free to add anything you feel is missing

Fair enough !!
I was just preparing a suggested tweak. I may as well complete it, and will post separately..

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.

4 participants