-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use closest year available for each map #374
base: master
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
for more information, see https://pre-commit.ci
src/compare_covermaps.py
Outdated
"Tigray2021": 2021, | ||
"Tigray2020": 2020, | ||
"Zambia2019": 2019, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really love that every time an intercomparison is made this file needs to be updated. I think it would be cleaner to minimize (or eliminate) changes to this file, and set the configuration (dataset name, admin code, year) in each respective notebook. In my view it'll be a bit more modular and pleasant to use that way.
Of course that script that generates all the intercomparisons would need to be updated or deleted, but since this is the primary way we are doing intercomparisons, I would be alright with that.
What do you think? @hannah-rae
@@ -174,13 +200,23 @@ def ee_script(self, country: str, include_export: bool = True, include_prefix: b | |||
}});""" | |||
return script | |||
|
|||
def extract_test(self, test: gpd.GeoDataFrame) -> gpd.GeoDataFrame: | |||
def extract_test(self, test: gpd.GeoDataFrame, year: int) -> gpd.GeoDataFrame: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this parameter be a list to allow sampling from a range of years?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a more explicit way of specifying which maps should or should not be sampled
src/compare_covermaps.py
Outdated
), | ||
"glad": Covermap( | ||
"glad", | ||
'ee.ImageCollection("users/potapovpeter/Global_cropland_2019")', | ||
resolution=30, | ||
probability=0.5, | ||
years_covered=[2003, 2007, 2011, 2015, 2019], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This collection only includes images for 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, this one is weird. I'll make it so that it changes the collection.
), | ||
"dynamicworld": Covermap( | ||
"dynamicworld", | ||
"""ee.ImageCollection( | ||
ee.ImageCollection("GOOGLE/DYNAMICWORLD/V1") | ||
.filter(ee.Filter.date("2019-01-01", "2020-01-01")) | ||
.filter(ee.Filter.date("2019-01-01", "2019-12-31")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this filter date thing is a bit messy now with the years. What do you think about making it a regex style in all instances.
.filterDate("YYYY-01-01", "YYYY-12-31")
and that YYYY is replaces with the year selected?
Also filterDate
is the same as .filter(ee.Filter.date
so it can be replaced everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that may work... the issue I had when thinking about this is that the Covermap objects get created before they are sampled. But as long as every dataset has a year specified then it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluating the ee_asset from a string can just be moved into extract_test to fix this:
ee_asset = eval(ee_asset_str.replace("\n", "").replace(" ", ""))
for more information, see https://pre-commit.ci
Updates:
These changes were made to ensure the
|
For maps that have more than one year available, we use the closest year to the test set year.
TODO: update results for all map notebooks