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

Enhancement of the hyint recipe to include etccdi indices #1133

Merged
merged 29 commits into from Apr 25, 2020

Conversation

earnone
Copy link
Contributor

@earnone earnone commented Jun 5, 2019

This version of hyint now includes the recipe_hyint_extreme_events with the hyint diagnostics in pipe with the extreme_events diagnostics. See #1063. This capability was already in the previous version of hyint but not called, so only a few refinements were applied.


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

  • 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
    New recipe/diagnostic

  • Add documentation for the recipe to the doc/sphinx/source/recipes folder and add a new entry to index.rst

  • Add provenance information


Closes #1063

@earnone earnone added the MAGIC label Jun 5, 2019
@earnone earnone added this to To do in MAGIC via automation Jun 5, 2019
@bouweandela
Copy link
Member

bouweandela commented Jun 14, 2019

Hi @earnone, is this something you are still working on, or is it ready for merging from your point of view?

@earnone
Copy link
Contributor Author

earnone commented Jun 17, 2019

@bouweandela it needs a little update to the provenance piped from extreme_events.

@mattiarighi mattiarighi moved this from To do to Testing in MAGIC Jul 25, 2019
@bouweandela
Copy link
Member

bouweandela commented Sep 12, 2019

@earnone Can you finalize this pull request? Then we can test it when you're done.

@bouweandela
Copy link
Member

bouweandela commented Oct 18, 2019

@earnone We recently started using the styler package to style all our R code. Of course this introduced some conflicts with the changes implemented here, which I just tried to fix. Can you please check if your changes are still as expected (and use the styler package from now on to style your code?).

@mattiarighi
Copy link
Contributor

mattiarighi commented Dec 19, 2019

@earnone is this still relevant?
If yes, please finalize, otherwise close it and remove the branch.

@earnone
Copy link
Contributor Author

earnone commented Dec 19, 2019

@mattiarighi mattiarighi changed the base branch from version2_development to master Jan 3, 2020
@earnone
Copy link
Contributor Author

earnone commented Jan 14, 2020

I am closing this and opening a new pull request from scratch starting from master.

@earnone earnone closed this Jan 14, 2020
@mattiarighi
Copy link
Contributor

mattiarighi commented Jan 14, 2020

Please also delete the branch, if not used.

@earnone earnone reopened this Mar 11, 2020
@earnone
Copy link
Contributor Author

earnone commented Mar 11, 2020

@bouweandela @mattiarighi I have reopened this to finalize the extreme_events option of hyint. No need for a new pull request. Solving conflicts with latest version and working on provenance in cascade from extreme_events (#1063)

@earnone
Copy link
Contributor Author

earnone commented Apr 14, 2020

All tests were successful. @bouweandela could you test it and review this? Thanks.

Copy link
Member

@bouweandela bouweandela left a comment

Looks good! Could you please also add documentation for the new recipe and give the pull request a descriptive title that I can use in the changelog when making a new release of ESMValTool?

@earnone earnone changed the title Version2 hyint extremes Enhancement of the hyint recipe to include etccdi indices Apr 16, 2020
@earnone
Copy link
Contributor Author

earnone commented Apr 16, 2020

@bouweandela The current recipe_hyint.rst in master already mentions the possibility of including additional indices from extreme_events. In fact, the recipe_hyint_extreme_events.yml is just as recipe_hyint.yml but with the further call to the extreme_events diagnostic and inclusion of its results in hyint.
I would like to avoid to cause confusion, would it be better to point to a joint documentation? Or do we want to duplicate everything and add the further specs for the extreme_events part?

@bouweandela
Copy link
Member

bouweandela commented Apr 16, 2020

In that case you could maybe update recipe_hyint.rst so it includes this recipe? E.g. mention it here: https://github.com/ESMValGroup/ESMValTool/blob/master/doc/sphinx/source/recipes/recipe_hyint.rst#available-recipes-and-diagnostics and add some explanation on what the difference between the two recipes is?

@earnone
Copy link
Contributor Author

earnone commented Apr 16, 2020

Yes, I'd prefer to keep all in one place. How does it work with the file recipe_hyint_extreme_events.rst? We simply don't create it?

@earnone
Copy link
Contributor Author

earnone commented Apr 18, 2020

The recipe_hyint.rst file was updated to cover also the options for recipe_hyint_extreme_events.yml (without creating a separate recipe_hyint_extreme_events.rst). This should complete the PR.

Copy link
Member

@bouweandela bouweandela left a comment

Looks good to me now, thanks! @mattiarighi Could you please test and merge when you're happy with this?

@mattiarighi
Copy link
Contributor

mattiarighi commented Apr 21, 2020

extreme_events.R stops because of a missing library:

Error in library(ncdf4.helpers) :
  there is no package called ‘ncdf4.helpers’
Execution halted

@earnone
Copy link
Contributor Author

earnone commented Apr 21, 2020

extreme_events.R stops because of a missing library:
Error in library(ncdf4.helpers) :`

Yes, this is the issue affecting extreme_events.R (lack of ncdf4.helpers library on CRAN and need to install it from archive) and the reason for keeping this hyint capability in stand-by. I understand we decided to proceed anyway while waiting for the library to be upgraded or extreme_events rewritten without the missing library?

@mattiarighi
Copy link
Contributor

mattiarighi commented Apr 21, 2020

Sorry, I missed the discussion.

Should I test without extreme_events.R for now and leave this to another PR?

@earnone
Copy link
Contributor Author

earnone commented Apr 21, 2020

Sorry, I missed the discussion.

Should I test without extreme_events.R for now and leave this to another PR?

Actually this improvement of hyint is solely related to ingesting the results from extreme_events, so there is nothing to test without that part (everything else is untouched). As you prefer, we can also keep this aside and not include it in master for now. @bouweandela?

@bouweandela
Copy link
Member

bouweandela commented Apr 21, 2020

@mattiarighi The problem is reported here #1443. I would suggest going ahead with testing and merging this pull request, because the figures are part of the paper and the problem is with an existing recipe, not so much with what is added in this pull request.

We can see how to proceed with the dependency problems later, either by re-implementing the indices or by helping the authors of the ncdf4.helpers package to get their package back on CRAN.

@mattiarighi
Copy link
Contributor

mattiarighi commented Apr 22, 2020

Dumb question: where can I get the `ncdf4.helpers' package to test this PR?

@earnone
Copy link
Contributor Author

earnone commented Apr 22, 2020

Dumb question: where can I get the `ncdf4.helpers' package to test this PR?

Here

And here is a suggestion on how to install from url:

# Download package tarball from CRAN archive
url <- "https://cran.r-project.org/src/contrib/Archive/ncdf4.helpers/ncdf4.helpers_0.3-3.tar.gz"
pkgFile <- "ncdf4.helpers_0.3-3.tar.gz"
download.file(url = url, destfile = pkgFile)
# Install package
install.packages(pkgs=pkgFile, type="source", repos=NULL)
# Delete package tarball
unlink(pkgFile)

@mattiarighi
Copy link
Contributor

mattiarighi commented Apr 23, 2020

Thanks, I have tested both recipes successfully.

Maybe you can add the above snippet to the recipe documentation until we solve #1443.

The library needs to be installed manually by users
@earnone
Copy link
Contributor Author

earnone commented Apr 23, 2020

@mattiarighi Could you have a look? Do we have a standard way to include known issues or warnings in the doc? I can replicate that in recipe_extreme_events too.

@mattiarighi
Copy link
Contributor

mattiarighi commented Apr 23, 2020

Thanks, it looks good to me.

I don't think we have a standard way of documenting this. Another option would be to add this to the recipe description section, it's more likely that users would look at that.

@bouweandela any opinion?

@bouweandela
Copy link
Member

bouweandela commented Apr 25, 2020

I think it's fine like this: we do not have a standard for documenting workarounds, because we try to just fix bugs instead. However, in this case that is taking some time, unfortunately.

@bouweandela bouweandela merged commit 9a60593 into master Apr 25, 2020
6 checks passed
@bouweandela bouweandela deleted the version2_hyint_extremes branch Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
MAGIC
  
Testing
Development

Successfully merging this pull request may close these issues.

Hyint with inclusion of ETCCDI indices from extreme_events_v2
4 participants