Skip to content

Attempt concatenation when cubes have overlapping data#280

Merged
mattiarighi merged 32 commits into
masterfrom
development_experiment_better_concatenation
Feb 11, 2020
Merged

Attempt concatenation when cubes have overlapping data#280
mattiarighi merged 32 commits into
masterfrom
development_experiment_better_concatenation

Conversation

@valeriupredoi

@valeriupredoi valeriupredoi commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

Better concatenation for two cubes that have time overlapping data and identical metadata - solves https://github.com/ESMValGroup/ESMValTool/issues/1329 and the olde issue with overlapping CMIP data (ie 1850-2000 and 1850-2300 both files in the same repo)

BTW whoever implemented the exp: list functionality *should have tested properly or added info to documentation, at the moment it's completely useless 🍺

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #349 and possibly (untested) #54

@valeriupredoi valeriupredoi added the enhancement New feature or request label Sep 26, 2019
@jvegreg

jvegreg commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

I was the one that added the documentation, I will do my penitence adding tests for this feature

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

hahaha no worries @jvegasbsc am almost finished writing tests 🍺

@jvegreg

jvegreg commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

It is already done

@jvegreg

jvegreg commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

I have to add #259 to make the tests pass, as I was using 0:00 hours in the time

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

sweeet! I was just about to commit, but not no more, cheers Javi 🍺

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

@jvegasbsc I fixed your fix, man 😁
The way was implemented was not working unfortunately, now it's all very schoolboy explicit for all possible cases. I removed an older test since we can do concatenation of overlapping cubes now; all your tests work; I also tested with real life data 🍺

@mathause mathause left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am a bit torn - it is nice functionality but quite magical - i.e. unexpected/ unwanted things can happen silently. It would be nice if the breakpoint can [optionally] be set by a keyword argument.

  • Example 1:
    - {dataset: MIROC5, project: CMIP5, mip: Amon, exp: [historical, rcp85], ensemble: r1i1p1, start_year: 1950, end_year=2099}
    This works beautifully - the historical data goes to 2012, rcp85 starts in 2006 & it concatenates the way I want - historical to 2005 and rcp85 from 2006.

  • Example 2:
    - {dataset: CMCC-CESM, project: CMIP5, mip: Amon, exp: [historical, rcp85], ensemble: r1i1p1, start_year: 1950, end_year=2099}
    Here, historical goes to 2005, but rcp85 starts in 2000. So I get historical until 1999 and rcp85 starts in 2000 & I don't even realize this happens.

Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread esmvalcore/preprocessor/_io.py
@valeriupredoi

Copy link
Copy Markdown
Contributor Author

@mathause cheers muchly for testing!! good to hear you are getting the (mostly) desired output, yeah, I'll have a look at the changes next week, off to pub now 🍺

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

cheers for suggestions @mathause - put them in. Given you tested (cheers a lot for that!), are you happy to go? 🍺

@mathause

mathause commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

Could not get to work right now, have to come back on monday...

Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread tests/integration/preprocessor/_io/test_concatenate.py
Comment thread tests/integration/preprocessor/_io/test_concatenate.py Outdated
Comment thread tests/integration/preprocessor/_io/test_concatenate.py Outdated
@bouweandela bouweandela changed the base branch from development to master January 3, 2020 12:18
@valeriupredoi

Copy link
Copy Markdown
Contributor Author

OK @bouweandela debug messages added - now it should be very clear what data is being used and how 🍺 Can we go ahead pls 🎖️

Comment thread esmvalcore/preprocessor/_io.py Outdated
Comment thread esmvalcore/preprocessor/_io.py Outdated
valeriupredoi and others added 4 commits February 3, 2020 12:14
Co-Authored-By: Bouwe Andela <bouweandela@users.noreply.github.com>
Co-Authored-By: Bouwe Andela <bouweandela@users.noreply.github.com>
@bouweandela

Copy link
Copy Markdown
Member

@valeriupredoi I made some minor changes, can you check if you're still happy with the result?

@bouweandela bouweandela left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mattiarighi Could you please test?

@valeriupredoi valeriupredoi left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good stuff, cheers @bouweandela - let's merge this then 🍺

f"ends before %s", cubes[0], cubes[1])
logger.debug(f"Cube %s contains all needed data "
f"so using it fully", cubes[1])
if data_end_1 <= data_end_2:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@valeriupredoi

Copy link
Copy Markdown
Contributor Author

@mattiarighi you wanna test pls mate? 🍺

@mattiarighi mattiarighi merged commit c4b82db into master Feb 11, 2020
@mattiarighi mattiarighi deleted the development_experiment_better_concatenation branch February 11, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request preprocessor Related to the preprocessor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

recipe: using exp: [historical, ssp126] with overlapping data

6 participants