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

BeninSmallHolderCashews: return geospatial metadata in sample #377

Merged
merged 13 commits into from
Feb 24, 2022

Conversation

recursix
Copy link
Contributor

exposing geoinformation in the return dict of getitem for benin_cashews.py

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Jan 29, 2022
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
@adamjstewart adamjstewart changed the title Geo information BeninSmallHolderCashews: return geospatial metadata in sample Jan 31, 2022
instead of storing it in self.
Adapt docstring
@ghost
Copy link

ghost commented Feb 1, 2022

CLA assistant check
All CLA requirements met.

torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
torchgeo/datasets/benin_cashews.py Outdated Show resolved Hide resolved
@adamjstewart adamjstewart added this to the 0.3.0 milestone Feb 2, 2022
@recursix
Copy link
Contributor Author

recursix commented Feb 3, 2022

Have the test run? Many of the are reported as "expected"

I'm still waiting to get approval to sign the CLA.

@adamjstewart
Copy link
Collaborator

For first time contributors GitHub requires maintainers to explicitly run the tests (just did). This is a security feature to prevent bitcoin mining on CI resources. We could disable this.

@adamjstewart
Copy link
Collaborator

I wouldn't both using autopep8, we use black and isort instead. See https://torchgeo.readthedocs.io/en/latest/user/contributing.html#linters if you haven't already.

@recursix
Copy link
Contributor Author

recursix commented Feb 3, 2022

I wouldn't both using autopep8, we use black and isort instead. See https://torchgeo.readthedocs.io/en/latest/user/contributing.html#linters if you haven't already.

I'm trying to avoid unconfiguring my environment and configuring it just for that PR.

@recursix
Copy link
Contributor Author

recursix commented Feb 9, 2022

Ok, I switched to black and installed pre-commit hooks

@adamjstewart
Copy link
Collaborator

@calebrob6 did we ever get the CLA issue resolved?

@calebrob6
Copy link
Member

Yes the CLA bot currently works I think

@recursix
Copy link
Contributor Author

recursix commented Feb 23, 2022

I was still waiting on the approval to sign the CLA (manager in vacation). Now it is signed.

Thanks.

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Can you replace all tile_transform with transform?

@recursix
Copy link
Contributor Author

will do. I'm about to leave on vacation. I might have to wait after.

@calebrob6
Copy link
Member

calebrob6 commented Feb 24, 2022 via email

@recursix
Copy link
Contributor Author

Awesome!

@adamjstewart adamjstewart merged commit 26b6917 into microsoft:main Feb 24, 2022
@adamjstewart
Copy link
Collaborator

Thanks for the PR @recursix! Hopefully we can get this converted to a GeoDataset in the future when all of us have more free time.

@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
…oft#377)

* exposing geoinformation

* restoring white spaces

* Returns tile_transform and crs
instead of storing it in self.
Adapt docstring

* adding types

* adjust line length

* fixing formatting

* remove auto-format from another auto-pep8 tool

* adjust order of import

* radjust autopep8

* passes pre-commit hook

* Add Oxford comma

* tile_transform -> transform

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants