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.

Sign up for GitHub

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

Jump to bottom

Implement land cover dataset #32

Merged
merged 22 commits into from
Nov 15, 2023
Merged

Implement land cover dataset #32

merged 22 commits into from
Nov 15, 2023

Conversation

geek-yang
Copy link
Member

Add ESA satellite land cover dataset to zampy.

@BSchilperoort
Copy link
Contributor

Perhaps we should get the landcover regridding method integrated into xarray-regrid, so we can regrid the land cover data. Otherwise we can't properly run a recipe with this dataset, right?

@geek-yang
Copy link
Member Author

Perhaps we should get the landcover regridding method integrated into xarray-regrid, so we can regrid the land cover data. Otherwise we can't properly run a recipe with this dataset, right?

Couldn't agree more. When working on this PR, I got the same thought. I think I will only implement the downloading function for now and wait a bit until the land cover regridding function is available in xarray-regrid, as the ingest function already requires regridding.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

54.5% 54.5% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@geek-yang geek-yang marked this pull request as ready for review October 19, 2023 12:18
geek-yang
geek-yang commented Oct 19, 2023
View reviewed changes
Copy link
Member Author

@geek-yang geek-yang left a comment

Choose a reason for hiding this comment

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

This PR is ready for review. Currently unit tests on windows platform are still failing because of the PermissionError. Does anyone have an idea how to tackle it? For the tests of other datasets, we had similar issue before but they were fixed by using mktemp (see this PR #10). But it becomes an issue again...I think this time it relates to the unzip.

SarahAlidoost
SarahAlidoost approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@geek-yang thanks 👍 looks good, just some suggestions/comments.

BSchilperoort
BSchilperoort approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

Just some very minor comments :)

src/zampy/datasets/land_cover.py Outdated Show resolved Hide resolved
src/zampy/datasets/land_cover.py Outdated Show resolved Hide resolved
src/zampy/datasets/land_cover.py Outdated Show resolved Hide resolved
src/zampy/datasets/land_cover.py Outdated Show resolved Hide resolved
tests/test_data/land-cover/land-cover_LCCS_MAP_300m_1996.zip Outdated Show resolved Hide resolved
@geek-yang @BSchilperoort
Apply suggestions from code review
0028595 
Co-authored-by: Bart Schilperoort <b.schilperoort@esciencecenter.nl>
@BSchilperoort BSchilperoort mentioned this pull request Nov 15, 2023
Merged
2 tasks
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

95.3% 95.3% Coverage
0.0% 0.0% Duplication

@geek-yang geek-yang merged commit 9d24eed into main Nov 15, 2023
12 checks passed
@geek-yang geek-yang deleted the implement-land-cover branch November 15, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BSchilperoort BSchilperoort BSchilperoort approved these changes

@SarahAlidoost SarahAlidoost SarahAlidoost approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geek-yang @BSchilperoort @SarahAlidoost