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

temporary fix to an issue with cube intersection when cube has bounds #800

Closed
wants to merge 3 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Sep 30, 2020

Before you start, please read our contribution guidelines.

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.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

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 #issue_number #799 and addresses an inherently iris issue firt posted in SciTools/iris#3391

@valeriupredoi valeriupredoi added bug Something isn't working iris Related to the Iris package preprocessor Related to the preprocessor labels Sep 30, 2020
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

I can confirm that this works, but I'm not keen on workarounds like this. Let's also try to get the issue fixed in iris.

esmvalcore/preprocessor/_area.py Outdated Show resolved Hide resolved
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
@Peter9192
Copy link
Contributor

I don't mind about codacy (all the more reason to get back to this at some point), but we should solve the flake8 error.

@jvegreg
Copy link
Contributor

jvegreg commented Oct 1, 2020

I can confirm that this works, but I'm not keen on workarounds like this. Let's also try to get the issue fixed in iris.

Neither am I. It will be possible to, at least, remove the bounds only if we need it and keep them unchanged in all the other cases? I am not keen on dumping metadata so easily

@valeriupredoi
Copy link
Contributor Author

yeah we could kill the bounds and re-guess them only if longitude has negative points, as we've seen the case, but that'll only add to the confusing nature of this kluge - I believe we should just have it as it is and immediately get rid of it when @bjlittle and the Iris team have fixed this in Iris 🍺

@ruthlorenz
Copy link

Are you guys sure this solves the issue?
the case was a region to extract from -10 to 39 longitude.
Without this fix the values in the preprocessed file I obtain are

lon = -1.25, 1.25, 3.75, 6.25, 8.75, 11.25, 13.75, 16.25, 18.75, 21.25, 23.75, 26.25, 28.75, 31.25, 33.75, 36.25, 38.75, 351.25, 353.75, 356.25

with the fix its
lon = 1.25, 3.75, 6.25, 8.75, 11.25, 13.75, 16.25, 18.75, 21.25, 23.75, 26.25, 28.75, 31.25, 33.75, 36.25, 38.75, 351.25, 353.75, 356.25, 358.75

it gets rid of the -1.25 which is good, but at the end it still jumps from 38.75 to 351.25.
So extract_region ends up adding the region west of the 0 meridian at the eastern boundary.

Or is this a second issue? that extract_region does not work properly for regions crossing the 0 meridian?

@jvegreg
Copy link
Contributor

jvegreg commented Oct 6, 2020

This is a known behaviour of Iris (I think @mwjury also experimented this). Although it is quite of weird when you check the file itself, it should not interfere with anything, I have seen it been interpreted correctly in diagnostics, but if it break yours we can look for a solution.

@mwjury
Copy link
Contributor

mwjury commented Oct 6, 2020

Yes, at first I was very confused when looking at the maps I produced, though it made sense looking at the data in ncview. But aside from that everything has been working correctly further downstream. Iris has it's problem with plotting such data, so the only solution was to center lon on zero in the diagnostics.

@ruthlorenz
Copy link

Thanks for the clarification. I do not think it breaks anything, just looks weird and makes the comparison to another implementation more difficult. We will keep this feature in mind ;-)

@valeriupredoi
Copy link
Contributor Author

just tested for the unwanted behavor with iris=3.0.1 (current version we use) and it's still there, reported in this comment - do we want to go ahead with this patch here (more of a kluge rather than a patch) or wait for the iris folk to sort it out? Asking since we need to do some maintenance on this branch if we want to go ahead with it 🍺

@Peter9192
Copy link
Contributor

I think it's a good idea to have this as a workaround, but we should be a bit more sophisticated about how to implement in. For example, we could add a test that triggers this error and mark it with xfail, linking to the iris issue or this one. Then, if it's fixed in iris, we should see it because the xfail test suddenly passes. I also still like the suggestion to do this only if strictly necessary.

@rcomer
Copy link

rcomer commented Mar 2, 2021

I think you could get the same effect using ignore_bounds=True in your call to intersection. It would save having to delete them.

@bouweandela
Copy link
Member

Looks like this may be fixed in iris soon now SciTools/iris#4046, nice work @rcomer!

@valeriupredoi
Copy link
Contributor Author

Looks like this may be fixed in iris soon now SciTools/iris#4046, nice work @rcomer!

yes, my apologies @rcomer - I got distracted and forgot to thank you for your work! 🍺

@bjlittle
Copy link
Contributor

bjlittle commented Mar 10, 2021

We're going to pull @rcomer's fix into v3.0.x and make it available in the forthcoming iris v3.0.2

HTH 👍

@rcomer
Copy link

rcomer commented Mar 10, 2021

Due to some terrifying conflicts, the fix has moved to a new PR: SciTools/iris#4059

@valeriupredoi
Copy link
Contributor Author

cheers muchly guys! we'll close this one when the iris work is released to the masses 😁

@valeriupredoi
Copy link
Contributor Author

fixed (and tested by meself) by the good people at Iris in SciTools/iris#4059

@valeriupredoi valeriupredoi deleted the kluge_fix_issue799 branch July 1, 2021 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working iris Related to the Iris package preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants