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

add CHELSA V2 #62

Merged
merged 22 commits into from
Jun 14, 2024
Merged

add CHELSA V2 #62

merged 22 commits into from
Jun 14, 2024

Conversation

tiemvanderdeure
Copy link
Contributor

This package currently downloads CHELSA data from V1.

CHELSA V2 has a bunch of additional variables, so I want to make those accessible from this package.

I think version 2 should become the default, but we need to still allow for version 1. The easiest way to do that would be to have version as a keyword argument for the CHELSA type

To-do list

@rafaqz
Copy link
Member

rafaqz commented Jan 30, 2024

Sure Chelsa version type parameter makes sense, 2 is assumed if not specified

@rafaqz
Copy link
Member

rafaqz commented Jan 30, 2024

Re test failures: the Aqua.jl keywords have changed. We'll need to remove the toml formatting part and also probably specify the Aqua.jl version in Project.toml

src/types.jl Outdated Show resolved Hide resolved
src/chelsa/shared.jl Outdated Show resolved Hide resolved
src/chelsa/bioclim.jl Outdated Show resolved Hide resolved
@tiemvanderdeure
Copy link
Contributor Author

Just need to add some tests and this is good to go.

For future climate, I haven't added a version keyword because CMIP6 is always version 2.

The bioclim variables are in the exact same path as the bioclim+ variables, so we could consider making it so that getraster(CHELSA{BioClim}) calls getraster(CHELSA{BioClimPlus}), and that will save us some lines of code.

@tiemvanderdeure
Copy link
Contributor Author

@rafaqz could you review this and help me troubleshoot these test failures?

@rafaqz
Copy link
Member

rafaqz commented Apr 15, 2024

Ok so Aqua.jl dosen't do toml formatting anymore, yoy can delete that line from the tests.

@rafaqz
Copy link
Member

rafaqz commented Apr 16, 2024

Aqua is in the runtest.jl file, near the top. While youre at it maybe add a line there to include("chelsa_climate.jl") ;)

@tiemvanderdeure
Copy link
Contributor Author

@rafaqz It now fails because of codecov. Could you take a look?

@rafaqz
Copy link
Member

rafaqz commented Apr 16, 2024

This looks good to go. Do you want to also update the readme table so CHELSA includes plus and climate?

(We can ignore that error its just codecov)

@tiemvanderdeure
Copy link
Contributor Author

FYI @rafaqz - precompilation gives this output. I can't figure out which line triggers this (nothing to do with this PR I think - it's the same on master)

Precompiling RasterDataSources
        Info Given RasterDataSources was explicitly requested, output will be shown live
┌ Warning: Replacing docs for `RasterDataSources.getraster :: Union{}` in module `RasterDataSources`   
└ @ Base.Docs docs\Docs.jl:243

@rafaqz
Copy link
Member

rafaqz commented May 10, 2024

Yeah would be nice to get a line number for that...

Fixed in #65

@rafaqz
Copy link
Member

rafaqz commented May 10, 2024

How can WorldClim downloads be this unreliable

@tiemvanderdeure
Copy link
Contributor Author

Are you kidding me, worldclim is down again!! I think this PR is done - can you re-trigger tests manually when worldclim is up again @rafaqz?

@rafaqz
Copy link
Member

rafaqz commented May 19, 2024

Yeah, maybe we need to cut any testing dependency on them or make it optional based on if it loads at all... Or maybe we can use GitHub actions cache somehow to cache the downloads now

It's so weird for such a widely used dataset.

@tiemvanderdeure
Copy link
Contributor Author

@rafaqz I think this PR is good to go - the tests just failed because worldclim was down. Can you manually re-run the tests? And maybe give the code a final glance?

@rafaqz rafaqz merged commit 37cf277 into EcoJulia:master Jun 14, 2024
2 of 4 checks passed
@rafaqz
Copy link
Member

rafaqz commented Jun 14, 2024

Thanks, great to have this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants