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

Updates of ocean diagnostics and recipes for GMD-2019-291 (ESMValTool Part II paper) #1621

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

ledm
Copy link
Contributor

@ledm ledm commented Apr 16, 2020

Before you start, read CONTRIBUTING.md and the guide for diagnostic developers.

Please discuss your idea with the development team before getting started, to avoid disappointment later. The way to do this is to open a new issue on GitHub. If you are planning to modify an existing functionality, please discuss it with the original author(s) by tagging them in the issue.


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)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. 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
  • Update documentation for the recipe to the doc/sphinx/source/recipes folder
  • Update provenance information if needed
  • Assign the author(s) of the affected recipe(s) as reviewer(s)

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 #1620

ledm added 30 commits June 21, 2019 16:47
…es to the recipe. Commented out models that need a fix.wq
…d multi-model mean to ecipe_ocean_scalar_fields.yml
…6' of github.com:ESMValGroup/ESMValTool into version2_development_ocean_development_for_v2_paper_1166
…6' of github.com:ESMValGroup/ESMValTool into version2_development_ocean_development_for_v2_paper_1166
@ledm
Copy link
Contributor Author

ledm commented May 22, 2020

This PR is now ready for review. Most of my plots listed here in GMD-2019-291 were already included.

This PR just adds some graphical improvements, a couple new time series diagnostics, and a new recipe.

@ledm ledm requested a review from valeriupredoi May 22, 2020 09:49
@bouweandela
Copy link
Member

Hi Lee,

Could you please clean up a bit by removing commented out lines from recipes and code? It would also be great if you could have a look at the failing tests on CircleCI. Let me know if you need help!

@ledm
Copy link
Contributor Author

ledm commented May 27, 2020

I've been testing recipe_ocean_amoc.yml from this recipe and it looks like something has changed with the extract_named_regions preprocessor. Line 570 of check.py:

cmor_points = [float(val) for val in coord_info.requested]

This line attempts to convert the requested dimensions into floats. This doesn't work with the strings required for this preprocessor and it fails.

@valeriupredoi
Copy link
Contributor

@jvegasbsc have a look at @ledm comment above please mate, it also seems a bit off to me at first glance too - there could be coords that have strings as points (like typesi) 🍺

@jvegreg
Copy link
Contributor

jvegreg commented May 27, 2020

I will take a look. It should not be too dificult

@ledm

This comment has been minimized.

@valeriupredoi
Copy link
Contributor

that's been fixed yesterday (and merged) - can you please merge master in here? 🍺

@ledm
Copy link
Contributor Author

ledm commented May 28, 2020

that's been fixed yesterday (and merged) - can you please merge master in here? 🍺

Cheers - solved it.

@jvegreg
Copy link
Contributor

jvegreg commented May 28, 2020

I will take a look. It should not be too dificult

Done!. Can you test if ESMValGroup/ESMValCore#657 solves your issue?

@mattiarighi
Copy link
Contributor

@valeriupredoi can you please test this?
It would be nice to have it merged for the final release.

@valeriupredoi
Copy link
Contributor

shall do! but I'd be happy if the conflicts get fixed first - they are scientific stuff and would feel uncomfortable fixing them meself 🍺

@@ -66,6 +45,7 @@ preprocessors:
scheme: linear_horizontal_extrapolate_vertical
area_statistics:
operator: mean
# fx_files: [areacello, ]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this commented out?

@tomaslovato
Copy link
Contributor

@ledm I might be able to spend some time in the coming weeks to revamp this stale branch if you think it is still valuable.
I spotted a previous branch in #1331 that looks to me like a precursor of the present one, as I saw similar changes e.g. in diagnostic_timeseries.py... is this correct or there are things to rescue also from #1331 ?

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.

Minor changes to ocean recipes & diagnostics for gmd-2019-291
6 participants