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

Lenient Cube Merge #4446

Open
1 task
bjlittle opened this issue Dec 2, 2021 · 12 comments
Open
1 task

Lenient Cube Merge #4446

bjlittle opened this issue Dec 2, 2021 · 12 comments
Labels
Dragon 🐉 https://github.com/orgs/SciTools/projects/19?pane=info Feature: Merge/Concatenate

Comments

@bjlittle
Copy link
Member

bjlittle commented Dec 2, 2021

✨ Feature Request

I'm keen to extend the lenient behaviour that underpins the Lenient Cube Maths introduced in iris 3.0, specifically to merge and also concatenate.

I'd be super interested to know whether there is appetite within the community for an (obvious) extension to such lenient behaviour within iris...

Motivation

iris is strict for obvious reasons regarding its treatment of metadata, but this is completely unhelpful when users require to combine multi-model data together in order to perform a common operation, such as a multi-model mean.

Due to the current metadata strictness of iris, this makes it close to impossible or incredibly painful for users, as iris forces the user to level the playing field with regards to their metadata before allowing them to proceed.

This is a burden on all our users, and that burden could be removed or significantly reduced by offering opt-in lenient merge and concatenate behaviours IMHO.

Please up vote this issue if you want to see this happen 😉

Edit 2023-07-27

The concatenate() aspect has been separated into #5392. While merge and concatenate are conceptually linked, the work can be entirely separated; indeed we could deliver one without the other. This allows for smaller parcels of work that are easier to find time for.

Tasks

  1. Dragon Sub-Task 🦎 Feature: Merge/Concatenate
@rcomer
Copy link
Member

rcomer commented Dec 2, 2021

There are more votes at #512, #2592 and #1987 😃

@bjlittle bjlittle pinned this issue Dec 2, 2021
@bjlittle bjlittle self-assigned this Dec 2, 2021
@bjlittle
Copy link
Member Author

bjlittle commented Dec 2, 2021

Perfect. Game on!

@bjlittle
Copy link
Member Author

bjlittle commented Dec 2, 2021

@SciTools/iris-devs Thinking about this, and also to align expectations... the next few weeks are maxed out with other project related work. Then it's the Christmas break. After much merriment, we're spinning up in January and dedicated to pulling together and releasing iris 3.2.

Even if we were to bank this right now I'd be nervous about pushing it into iris 3.2. IMHO this change needs serious development usage and testing before landing in a release. It'll also need appropriate user guide docs support, worked examples et al akin to lenient maths to ensure crystal clear community understanding.

That all said, for me the earliest this lands is iris 3.3... or am I over-estimating this?

@bjlittle
Copy link
Member Author

bjlittle commented Dec 2, 2021

Also, during lenient maths it was clear to me that this was an obvious next step. However, unlike lenient maths I believe this will be an additive feature and won't break them API, thanks to the common metadata API already being in place.

However, the main question here is whether lenient merge behaviour is enabled by default?

My answer to this is, yes it is. That's my starting position here. This would also align with the default behaviour of lenient maths.

If people aren't comfortable with this then now we have it landing in iris 4.0 and not 3.3.

And if people are adamant this is a breaking change, then we'd better also commit to delivering lenient concatenate as well, otherwise we'll soon have iris 5.0 on our hands.

Personally, I'm not phase by major releases, but two major releases e.g., 4 then 5, back to back smacks of something seriously weird going on within a package.

Thoughts? 🤔

@bjlittle
Copy link
Member Author

bjlittle commented Dec 3, 2021

Also, for me this work means adopting the common metadata API for merge (and concatenate).

Therefore my expectation is that the hidden benefit of this will be that we have a richer framework to report to the user exactly why cubes that they expect to merge don't - now that would be welcomed, I'm sure

@bjlittle bjlittle added this to the v3.3.0 milestone Dec 3, 2021
@bjlittle
Copy link
Member Author

bjlittle commented Dec 3, 2021

Are there any users out there who have some spare cycles and are willing to be early adopters for this functionality?

Basically, what this will entail is playing/using development versions of the functionality (when prompted) and feeding back.

Your feedback will be priceless ♥️

It'll also give us the confidence that any proposed changes to merge are fit for purpose, and it'll give us greater confidence when this functionality lands in a release that we've not broken peoples workflows.

If you're up for a bit of that, then up vote this comment 😄

@rcomer
Copy link
Member

rcomer commented Dec 3, 2021

However, the main question here is whether lenient merge behaviour is enabled by default?

My answer to this is, yes it is. That's my starting position here. This would also align with the default behaviour of lenient maths.

I agree. I think it would be right confusing for users if leniency was the default for arithmetic but not for other operations.

And if people are adamant this is a breaking change…

I’m probably missing something about what technically constitutes a breaking change but, as a user, if my code that currently works still works with lenient merge switched on, I don’t think I’d be worried.

@wjbenfold
Copy link
Contributor

I like this. If we get the opportunity to overhaul merge/concatenate in the future, then I think this should be part of it, if we don't and this is still achievable then I don't think we should let the future goal of overhauling merge/concatenate stop us.

I guess it's breaking behaviour if people are relying on spotting metadata mismatches by whether things merge (which probably isn't an explicit part of their workflow but might be an implicit one). If this had the chance to fold into a larger merge/concatenate revamp then the question becomes moot because that would presumably redefine the API.

I agree that jumping two major versions would probably be too much.

@bouweandela
Copy link
Member

This would be a useful feature for the ESMValTool community too.

@nhsavage
Copy link
Contributor

nhsavage commented Jun 7, 2022

one more use case for this is when using NWP data from multiple forecasts (for example joining the first 24 hours of each daily forecast to get a timeseries). This causes the files to have different model forecast_period and forecast_reference_time coordinates (aux coord and scalar respectively). these need to be removed to allow them to merge to a single cube.

@pp-mo
Copy link
Member

pp-mo commented Jan 11, 2023

Another possibly relevant question arose here
Worth considering here : Would we be fixing that?

@bjlittle
Copy link
Member Author

bjlittle commented Feb 28, 2023

Also see #5131 (@afinnen)

@trexfeathers trexfeathers assigned bjlittle and unassigned bjlittle Apr 12, 2023
@trexfeathers trexfeathers added the Dragon 🐉 https://github.com/orgs/SciTools/projects/19?pane=info label Jul 4, 2023
@trexfeathers trexfeathers assigned acchamber and unassigned pp-mo Jul 10, 2023
@trexfeathers trexfeathers removed this from the Candidate for next release milestone Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dragon 🐉 https://github.com/orgs/SciTools/projects/19?pane=info Feature: Merge/Concatenate
Projects
Status: 📌 Prioritised
Status: Parent Issue
Status: No status
Status: 🆕 New - potential tasks
Development

No branches or pull requests

9 participants