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

Fix for FGOALS-f3-L sftlf #667

Merged
merged 8 commits into from
Jan 7, 2021
Merged

Fix for FGOALS-f3-L sftlf #667

merged 8 commits into from
Jan 7, 2021

Conversation

mwjury
Copy link
Contributor

@mwjury mwjury commented Jun 5, 2020

Before you start, please read CONTRIBUTING.md.

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.


Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

a couple changes to make the fix lazy; what's the problem that is raised when running the fix?

esmvalcore/cmor/_fixes/cmip6/fgoals_f3_l.py Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip6/fgoals_f3_l.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cmip6/fgoals_f3_l.py Outdated Show resolved Hide resolved
@valeriupredoi valeriupredoi changed the title initial commit Fix for FGOALS-f3-L sftlf Jun 9, 2020
@bouweandela bouweandela added the fix for dataset Related to dataset-specific fix files label Jun 15, 2020
@bouweandela bouweandela requested a review from zklaus June 15, 2020 07:45
@mwjury
Copy link
Contributor Author

mwjury commented Jun 17, 2020

@valeriupredoi Thanks for the code review.
The main problem I have is that the fix does not get applied and hence the error mentioned in #667 gets thrown. I am using the sftlf data for the mask_landsea, but it does not get corrected.

@valeriupredoi
Copy link
Contributor

right - the problem is that currently the mask files that go to mask_* preprocessors don't get fixed on the fly - the only code patch that does that is in #439 but that is awfully off-track from master - for now the only suggestion I have is to use sftlf as a regular variable (hence your fix will be applied in that case) and use its (fixed) file in the diagnostic rather than in the preprocessor - can you do that or is it off limits of your workflow? 🍺

@mwjury
Copy link
Contributor Author

mwjury commented Jun 17, 2020

@valeriupredoi doh, thanks for clarifying!
Nah, that would mean a very long extra way I'd have to go in the diagnostics. Any estimate yet as to when #439 will be merged? Maybe it's best to wait a bit.

@valeriupredoi
Copy link
Contributor

#439 will never be merged, it'll die alone 😁 we are waiting for iris 3.0 to have a fully functional cell measures handling implemented so fx data can be handled seamlessly by them - these fixes are still useful though; you could fix the data via asking for sftlf as a regular variable, save the fixed files and then use them with a custom data input dir for the actual run; twisty two-stage pipeline but will get you to where you need to be 🍺

@mwjury
Copy link
Contributor Author

mwjury commented Jun 17, 2020

haha, I see. Thanks for the hint, let's see if I can may this work (or the modellers are changing the data first).

@valeriupredoi
Copy link
Contributor

modellers changing data is a very long wheelbase truck as @zklaus would confirm 😁

@mwjury mwjury requested a review from jvegreg as a code owner January 5, 2021 13:42
@jvegreg
Copy link
Contributor

jvegreg commented Jan 5, 2021

The codacy issue is added by yapf, so I left it alone

@jvegreg
Copy link
Contributor

jvegreg commented Jan 5, 2021

@valeriupredoi , I think we can merge this now, no?

@valeriupredoi
Copy link
Contributor

@jvegasbsc be my guest 👍

@jvegreg jvegreg merged commit c87a178 into master Jan 7, 2021
@jvegreg jvegreg deleted the fgoals_f3_l_sftlf_fix branch January 7, 2021 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants