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

Fix forced int32 type conversion in RasterDataset #384

Merged
merged 5 commits into from
Feb 17, 2022

Conversation

tritolol
Copy link
Contributor

@tritolol tritolol commented Feb 1, 2022

Fixes #379

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Feb 1, 2022
@tritolol tritolol changed the title fix forced int32 type conversion in RasterDataset Fix forced int32 type conversion in RasterDataset Feb 1, 2022
@isaaccorley
Copy link
Collaborator

isaaccorley commented Feb 1, 2022

This looks fine to me. I think the only thing we need to be careful of (and I've run into this in the past) is that pytorch doesn't support a uint16 data type and some geospatial imagery comes in this dtype. So the next line converting to a torch tensor would raise an error.

Edit: the tests caught exactly this error. We should probably check if the numpy arrays are uint16 or uint32 and convert to int32.

@adamjstewart
Copy link
Collaborator

@isaaccorley was there also some kind of dtype issue with Kornia or was that only with the number of channels?

@isaaccorley
Copy link
Collaborator

Yeah but the kornia thing will have to happen at the datamodule level, if you're referring to the masks required to be float dtype.

@adamjstewart
Copy link
Collaborator

Yep, just want to make sure we update those if needed too.

@isaaccorley
Copy link
Collaborator

Good point. I think it should be fine. Once the updates to this PR get made, if it breaks any tests we can figure it out then.

@tritolol
Copy link
Contributor Author

tritolol commented Feb 2, 2022

Edit: the tests caught exactly this error. We should probably check if the numpy arrays are uint16 or uint32 and convert to int32.

uint32 should rather be converted to int64 to prevent misinterpretation of high values, right?

@adamjstewart adamjstewart added this to the 0.2.1 milestone Feb 2, 2022
adamjstewart
adamjstewart previously approved these changes Feb 2, 2022
adamjstewart
adamjstewart previously approved these changes Feb 2, 2022
@adamjstewart
Copy link
Collaborator

It looks like we don't have any existing RasterDatasets that use np.uint32. We'll have to add a fake dataset (or modify the dtype of an existing dataset) so that we can get 100% test coverage.

@ghost
Copy link

ghost commented Feb 15, 2022

CLA assistant check
All CLA requirements met.

@github-actions github-actions bot added the testing Continuous integration testing label Feb 15, 2022
@calebrob6
Copy link
Member

calebrob6 commented Feb 15, 2022

Thanks for the contribution @tritolol! I just added a test case for when raster data is uint32 so we can keep 100% test coverage.

I think the only pending thing is for you to sign the CLA above.

@tritolol
Copy link
Contributor Author

Thanks for the contribution @tritolol! I just added a test case for when raster data is uint32 so we can keep 100% test coverage.

I think the only pending thing is for you to sign the CLA above.

Thank you for this useful repository. I hope that I find the time to contribute more in the future.
I tried to sign the CLA, unfortunately I only see this page where I can't do anything (tried with chrome and safari).
image

@calebrob6
Copy link
Member

I'm seeing this problem too, I'll find who to ask and @ you when its fixed

@calebrob6
Copy link
Member

@tritolol it was a recently introduced problem with the bot that was just rolled back -- do you mind trying again? (I'm seeing that the CLA page loads after ~10ish seconds)

@tritolol
Copy link
Contributor Author

@calebrob6 now it worked!

@calebrob6 calebrob6 merged commit daecc90 into microsoft:main Feb 17, 2022
adamjstewart pushed a commit that referenced this pull request Mar 19, 2022
* fix forced int32 type conversion

* add fix for numpy dtypes which are not supported by tensors

* delete whitespace

* Adding custom data to test the dtype transform

* Fixed formatting

Co-authored-by: Caleb Robinson <calebrob6@gmail.com>
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* fix forced int32 type conversion

* add fix for numpy dtypes which are not supported by tensors

* delete whitespace

* Adding custom data to test the dtype transform

* Fixed formatting

Co-authored-by: Caleb Robinson <calebrob6@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 testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for float type raster datasets
4 participants