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

Use cell area for weights if available #3173

Conversation

znicholls
Copy link
Contributor

@znicholls znicholls commented Sep 17, 2018

This PR addresses #3169, allowing the user to use the weights provided in the cell measure more easily.

As mentioned in #3169, at the moment it's not that clear how to use cell measures for these weights. This is my attempt at doing so. It has a number of problems which I need help to address

  • copying of code from iris.analysis.cartography.area_weights
  • where the decision to use the area weights should go (feels wrong in iris.analysis.cartography.area_weights but is awkward to put in iris.cube)
  • how cell measure should behave after collapsing (should cell area still be there, should it be the total area, the weighted average area?)
  • how cell measure should appear in representation (I've made a change but not sure if it's what is desired)
  • other PR bits e.g. docs, changelog

@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 17, 2018
@znicholls znicholls force-pushed the use-cell-area-for-weights-if-available branch from c18bb8e to eae56ea Compare September 17, 2018 10:34
@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 17, 2018
@DPeterK DPeterK removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 28, 2018
@znicholls znicholls changed the title WIP: Use cell area for weights if available Use cell area for weights if available Sep 29, 2018
@znicholls
Copy link
Contributor Author

@dkillick if you have a minute, it would be great to get a few pointers about where to go with this. At the moment it's a feature I'd love to have in iris (rather than just wrapping around as is my current workwaround) so I'm happy to put some time into getting it up to scratch, there's just a few areas where I really don't know how best to proceed.

@pp-mo pp-mo self-requested a review October 8, 2018 16:50
@pp-mo
Copy link
Member

pp-mo commented Oct 8, 2018

Sorry to be so slow.
This definitely is interesting -- we'll get round to it, promise !

@znicholls
Copy link
Contributor Author

Sorry to be so slow.

No stress. There's also no rush for me. I've started working on a bunch of Iris wrappers for use with simple climate models (see here). Hence I can put all the stuff I need right away there and then open pull requests like this for the bits that seem like they would better fit in the main repository than the wrapper repository.

@bjlittle
Copy link
Member

Hey @znicholls, how's it going?

Just pinging you on this PR to check on the state of play. Is this still an active WIP? Where you at?

Thanks 😄

Still needs to be cleaned

Also not sure what is meant to happen with the cell measure after collapsing.
It is causing issues for representation (scalar cell measure not yet handled)
and it is not clear what the behaviour should be. Should the cell measure
simply disappear, should we keep the average cell measure, the total cell
measure?
@znicholls znicholls force-pushed the use-cell-area-for-weights-if-available branch from eae56ea to c994b5b Compare October 22, 2019 23:55
@znicholls
Copy link
Contributor Author

Hey @bjlittle, yes still happy to have a go at this. As I mentioned earlier in the discussion, I just need some pointers on how best to add this feature. I've only added one test at the moment, is this enough? (I think there's also some code in there related to making html representations of cubes behave a bit nicer which I should remove and put in a different PR).

@znicholls
Copy link
Contributor Author

hi @bjlittle, is it worth revisiting this or have things moved in Iris since? I'm fiddling around with iris 3.0 and the html representation issue is certainly fixed!

@schlunma
Copy link
Contributor

@znicholls any plans to continue with this?

I didn't notice this pull request earlier and started implemented my own slightly different solution for this here. See also #5082.

@znicholls
Copy link
Contributor Author

No plans to continue @schlunma, your solution looks much more elegant so please go for it! If you want to take the test I wrote and see if your solution passes that too, that would be great but no pressure.

@znicholls znicholls closed this Nov 24, 2022
@znicholls znicholls deleted the use-cell-area-for-weights-if-available branch November 24, 2022 09:26
@schlunma
Copy link
Contributor

Awesome! I think it won't pass the test, since my solution still needs the user to explicitly set weights='cell_area', whereas yours uses the cell measure if possible even without the weights keyword (as far as I can tell) 👍

@pp-mo
Copy link
Member

pp-mo commented Nov 24, 2022

Awesome! I think it won't pass the test, since my solution still needs the user to explicitly set weights='cell_area', whereas yours uses the cell measure if possible even without the weights keyword (as far as I can tell) 👍

TBH now I've seen it I rather prefer that explicit keyword approach.
You could use Cube._dimensional_metadata to select a matching cell-measure/aux-coord/ancillary-variable by name, which possibly may be a useful generalisation. This has the advantage of automatic dimension mapping between the cube and its component, as shown in the code here.
We should also probably allow passing in such a thing (i.e. any DimensionalMetadata), but in that case we will have to take on trust that its dimensions map to the cube in the expected way -- so effectively that case is "just an array".
By contrast, I don't really favour automatically calculating area-weights. In those cases, I think it is "more Iris" to leave it to the user to provide what they they think is right.

These are just opinions !

@znicholls
Copy link
Contributor Author

znicholls commented Nov 24, 2022 via email

@schlunma
Copy link
Contributor

TBH now I've seen it I rather prefer that explicit keyword approach.

Fully agree!

You could use Cube._dimensional_metadata to select a matching cell-measure/aux-coord/ancillary-variable by name, which possibly may be a useful generalisation. This has the advantage of automatic dimension mapping between the cube and its component, as shown in the code here. We should also probably allow passing in such a thing (i.e. any DimensionalMetadata), but in that case we will have to take on trust that its dimensions map to the cube in the expected way -- so effectively that case is "just an array".

Thanks for the pointer to Cube._dimensional_metadata, that's really helpful! Is there also a similar function to access to associated dimensions mappings? I.e. something that generalizes Cube.coord_dims, Cube.cell_measure_dims, etc.?

By contrast, I don't really favour automatically calculating area-weights. In those cases, I think it is "more Iris" to leave it to the user to provide what they they think is right.

Fully agree as well!

@pp-mo
Copy link
Member

pp-mo commented Nov 24, 2022

Is there also a similar function to access to associated dimensions mappings? I.e. something that generalizes Cube.coord_dims

Yes, sort-of. All of the coords/cell-measures/ancillary-variable classes provide a cube_dims method.
( Its actually an abstract method of _DimensionalMetadata : They all have one, with the same signature, but there is no common implementation )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants