Skip to content

1.4.0 changes and what's new#540

Merged
esc24 merged 1 commit intoSciTools:v1.4.xfrom
bblay:suuup
Jun 6, 2013
Merged

1.4.0 changes and what's new#540
esc24 merged 1 commit intoSciTools:v1.4.xfrom
bblay:suuup

Conversation

@bblay
Copy link
Contributor

@bblay bblay commented Jun 5, 2013

No description provided.

@bblay
Copy link
Contributor Author

bblay commented Jun 5, 2013

Note: I removed the features and bugs headings because I couldn't determine which bugs to include. That made it look like we hadn't fixed any bugs! Hopefully, with imminent, stricter PR guidelines on updating these files as we go, that should not be a problem with future releases.

Copy link
Member

Choose a reason for hiding this comment

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

Why have you removed these entries? They are the result of the few PRs that remembered to add entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still in the changelog, but because it relates to the tests, i.e it's a developer issue, it doesn't belong in the what's new list.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree. It is a change to the configuration that directly affect some of our users. The changelog is not part of our built documentation so you can't expect that to be a sufficient route of communication. Let me be clear - changes such as these need to be easy to find. Sysadmins and developers who have custom site.cfg files need to be informed of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, it is there, further down: https://github.com/SciTools/iris/pull/540/files#L1R33
needs to have the new alternative stated too though. will do that now.

@esc24
Copy link
Member

esc24 commented Jun 5, 2013

I quite like the new format of CHANGES, but the short one-liners need some minor tidying. However, the What's New entries are far too brief in my opinion. Bear in mind the people reading this may not be familiar with the context and it's also our opportunity to highlight our achievements over the last few months.

Did we really not deprecate anything?

@ajdawson
Copy link
Member

ajdawson commented Jun 5, 2013

I agree with @esc24 the What's New page should be more detailed. Check out matplotlib's What's New page: http://matplotlib.org/users/whats_new.html, which I think is a nice example. Not every feature needs a huge amount of detail, but they should at least be clear what the feature is for unfamiliar users.

@bblay
Copy link
Contributor Author

bblay commented Jun 5, 2013

More details what's new

Ok, will do.

Did we really not deprecate anything?

That's not easy to answer just by trawling through the commit history.
I'll have another look through and see if I can see anything.

@ajdawson
Copy link
Member

ajdawson commented Jun 5, 2013

I know for sure that the add_custom_season_* functions from coord_categorisation.py have been deprecated in favour of adding their functionality to the add_season_* functions.

@bjlittle
Copy link
Member

bjlittle commented Jun 5, 2013

@bblay it might be worth while searching the code base on deprecated and you'll find a hit in coord_categorisation.py as @ajdawson said. We should be using the .. deprecated:: X.X Sphinx directive to document deprecated functionality.

Can't help feeling that we should be more strict with ourselves when we add new functionality so that the appropriate whats new files are updated as part of the PR review exit citerion. It might make life easier when it comes to a release. Thoughts ...

Copy link

Choose a reason for hiding this comment

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

this requires a more in-depth description

Copy link
Member

Choose a reason for hiding this comment

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

See the PR description #475 ... that might help clarify.

@bblay
Copy link
Contributor Author

bblay commented Jun 5, 2013

...we should be more strict with ourselves when we add new functionality so that the appropriate whats new files are updated as part of the PR...

@bjlittle, please could I encourage you to pop a comment somewhere on here

@bblay
Copy link
Contributor Author

bblay commented Jun 5, 2013

Please could people take a quick look at the what's new page, to check it is a fair representation of the work you wrote/reviewed. Many thanks, @munslowa, @esc24, @bjlittle, @pp-mo, @ajdawson, @cpelley.

Copy link
Member

Choose a reason for hiding this comment

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

I'd focus on the positive here. This is a big step forward in functionality. I'd also be tempted to combine the two experimental regridding features under a single section. I'll post some suggested text.

Copy link
Member

Choose a reason for hiding this comment

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

Experimental regridding enhancements

Bilinear and area weighted regridding functions are now available in :mod:iris.experimental.regrid. The final API is still in development, but in the meantime :func:regrid_bilinear_rectilinear_src_and_grid can be used to regrid a cube onto a horizontal grid defined in a different coordinate system. The data values are calculated using bilinear interpolation. :func:regrid_area_weighted_rectilinear_src_and_grid can be used to regrid a cube such that the data values of the resulting cube are calculated using the area weighted mean. Both of these function support masked data and handle derived coordinates such as hybrid height.

@ajdawson
Copy link
Member

ajdawson commented Jun 5, 2013

Deprecations are missing from the changelog.

Copy link
Member

Choose a reason for hiding this comment

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

"expoerted" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

um, it means taking all the poetry out! :D

@esc24
Copy link
Member

esc24 commented Jun 5, 2013

CHANGES has a release date, but what's new doesn't.

Copy link

Choose a reason for hiding this comment

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

I do not believe this description covers all changes made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpelley, could you elaborate please?

@pp-mo
Copy link
Member

pp-mo commented Jun 5, 2013

Checking stuff I was involved with, items seem missing ?

(*) No mention of Grib translation extensions - #482.
Levels change is mentioned, but not ...

  • more grib2 params recognised on input
  • some grib2 params may now read with different standard_name
  • now translates some codes on grib2 output

(*) No mention of ESMPy conservative regridding - #453
Suggest something like

experimental support for area-conservative regridding between geographical coordinate systems.
This uses the ESMF library functions, via the ESMPy interface

@pp-mo
Copy link
Member

pp-mo commented Jun 5, 2013

I also think the regridding extensions should be grouped together.
There are 3, I think --

I reckon the comparison would make it clearer what each one does.

@bblay
Copy link
Contributor Author

bblay commented Jun 5, 2013

Many thanks for taking a look, peeps. 👀
All addressed except @cpelley's comment that the cube merge description is incomplete, awaiting further information.

@ghost ghost assigned bblay Jun 5, 2013
@cpelley
Copy link

cpelley commented Jun 6, 2013

#475

This PR also resolves the situation when a NotYetImplementedError exception was raised when the situation of No functional relationship between separable and inseparable candidate dimensions was determined. Instead, the candidate coordinates participating within this group are all enumerated along an anonymous dimension.

Since users might be familiar with the exception that was raised rather than the reasons behind it, it might be worth mentioning the scenario for this. Just a thought really, I wouldnt hold anything back for it!

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

it might be worth mentioning the scenario for this

If someone can offer a suggested text I'll add it. That quote is gobbledygook 😃
I'll also have a little look at the code myself but I suggest we leave this one.

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

I suggest:

"Cube merging now favours numerical coordinates over string coordinates
when identifying a coordinate to describe a dimension. Furthermore, it now favours :class:~iris.coords.DimCoord over :class:iris.coords.AuxCoord. These modifications reduce the likelihood of the error: No functional relationship between separable and inseparable candidate dimensions was determined."

@cpelley
Copy link

cpelley commented Jun 6, 2013

👍

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

(Perhaps we should merge this and discuss the changelog link elsewhere)

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

I can't find the error it refers to

See https://github.com/SciTools/iris/blob/v1.3.0/lib/iris/_merge.py#L687

I got the wording slightly wrong. Please copy it from the above link.

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

Please make the release dates consistent in changes and what's new. I suggest tomorrow i.e. 07 June 2013

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

That's because the fix fixed it.

Copy link
Member

Choose a reason for hiding this comment

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

calculaus -> calculus

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

All review actions complete, except the cube merge text because I still can't find the error it refers to.

Copy link
Member

Choose a reason for hiding this comment

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

Move this "Both ..." up to the top paragraph.

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

That's because the fix fixed it.

Hehehe, ok, then we can't really say, "These modifications reduce the likelihood of the error: No functional relationship between separable and inseparable candidate dimensions was determined."

May we drop this one?

Copy link
Member

Choose a reason for hiding this comment

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

This returns a cube list. Either use cubes = ... or use load_cube()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot, thanks.

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

May we drop this one?

OK by me but @cpelley wanted something so I suggest adding "These modifications prevent the error: No functional relationship between separable and inseparable candidate dimensions."

@esc24
Copy link
Member

esc24 commented Jun 6, 2013

@bblay - I'm done. Please see comments above.

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

Thanks, I think everything's been addressed.

@cpelley
Copy link

cpelley commented Jun 6, 2013

I'm happy to let it be, I only though that users who were familiar with this message for their case would identify the changes as being relevant to them, but its not important enough to hold this PR for.

CHANGES Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Propogate -> Propagate

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

@cpelley, it made it in: bblay@fe9b83a#L1R144

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

(I think you were both right to push for it)

CHANGES Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Change to:
"Improved error message within cube.aggregated_by()"

or similar

@bblay
Copy link
Contributor Author

bblay commented Jun 6, 2013

Thanks for the high quality review work, looking really good now.

@cpelley
Copy link

cpelley commented Jun 6, 2013

👍

esc24 added a commit that referenced this pull request Jun 6, 2013
1.4.0 changes and what's new
@esc24 esc24 merged commit fa45e64 into SciTools:v1.4.x Jun 6, 2013
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.

6 participants