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

Add band selection to So2Sat Dataset and adopt plot method #394

Merged
merged 2 commits into from
Feb 20, 2022

Conversation

nilsleh
Copy link
Collaborator

@nilsleh nilsleh commented Feb 11, 2022

This PR addresses the last comment by Adam in issue #289 about adding band selection to the So2Sat Dataset. Changes I made:

  • add band selection as a sequence and select desired bands in getitem
  • adopt the plot method to look for RGB indices and only plot then
  • since the dataset takes a specific split as an argument, I thought it would make sense that the integrity check also only checks the specific selected split, so that not all splits need to have been downloaded

The remaining question I have is whether there is a standard naming convention for the S1 bands, described in the dataset repo. I couldn't find one and therefore just prepended "S1".

Plot examples:
s021
s022
s023

@nilsleh nilsleh marked this pull request as draft February 11, 2022 11:15
@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing labels Feb 11, 2022
@calebrob6
Copy link
Member

The remaining question I have is whether there is a standard naming convention for the S1 bands, described in the dataset repo. I couldn't find one and therefore just prepended "S1".

This seems fine to me!

@nilsleh nilsleh marked this pull request as ready for review February 14, 2022 18:39
@adamjstewart adamjstewart added this to the 0.3.0 milestone Feb 18, 2022
@calebrob6 calebrob6 merged commit 89277dc into microsoft:main Feb 20, 2022
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
@adamjstewart
Copy link
Collaborator

I'm looking back at this and I'm not sure if I like this solution anymore. I think it makes sense to allow uncurated raster datasets like Landsat8 and Sentinel2 to support band selection, but I'm not sure if it makes sense to allow this level of customization for curated benchmark datasets like So2Sat. Is anyone really going to want to evaluate the performance of their model on So2Sat Sentinel-2 bands only but with a subset of the bands, or the bands in a different order? I think it would make more sense to get rid of the bands parameter and instead have a band_set parameter that takes in values like "all", "s1", "s2", "rgb", etc. What do you think? This level of customization still allows us to use So2Sat to create pre-trained models for only Sentinel-1 or Sentinel-2 data (although not with all s2 bands).

P.S. The band names chosen in this PR don't match the band names in our Sentinel2 class, they probably should.

yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
…#394)

* add band selection and adopt plot method

* np array typing and raise error doc
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.

3 participants