-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Added sample for geospatial classification #7291
Added sample for geospatial classification #7291
Conversation
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.
Hi Nikita! Thanks for taking the time to collaborate with this sample, we really appreciate it!
This is looking great! I made some comments, mostly on style and nitpicking.
Some background: I learned C and C++ in college and I grew writing imperative code, but the more that I've been delving into purely functional languages (like Elm and Haskell), I've been learning really cool ways of doing the same things but without side effects like mutating variables. I've noticed it's usually shorter, more concise, easier to read, and less error prone, but it does require a slightly different way of doing things. I think in summary, avoiding to mutate variables, avoiding for-loops and while-loops, and using list comprehensions or functions instead. I've reinforced this with Beam / Dataflow, Earth Engine, and even TensorFlow Datasets, where everything is done via
map
,filter
, and some kind ofreduce
, and it makes it easier to parallelize / distribute.
people-and-planet-ai/geospatial-classification/requirements.txt
Outdated
Show resolved
Hide resolved
people-and-planet-ai/geospatial-classification/requirements-test.txt
Outdated
Show resolved
Hide resolved
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 is looking great! I left some minor comments on the notebook. In general, can we keep the cell outputs of the code cells? That way people can skim through the notebook and see the outputs without having to actually run it. Although we should remove any cell output that includes personal information like a project ID or a bucket name.
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.
LGTM, I'll be OOO next week, but this is looking great!
Thanks David! Just resolved your last few comments :) |
All tests are passing, I think this should be ready to merge. Thanks @nikitamaia for contributing in this lovely sample! |
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.
Please add type hints or, if for some reason an underlying product doesn't support that, make a note in the noxfile config and set the "enforce" boolean to false
"ignored_versions": ["2.7", "3.6", "3.7", "3.9", "3.10"], | ||
# Old samples are opted out of enforcing Python type hints | ||
# All new samples should feature them | ||
# "enforce_type_hints": True, |
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.
We do ask that new samples use type hints as per the authoring guide https://github.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#documenting-types - please switch that to true and add them in
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 made some minor comments while running through the sample. Other than that, LGTM
Description
Geospatial classification sample for the People + Planet AI series.