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

Sentinel-2: use 10,000 as scale factor #1027

Merged
merged 9 commits into from
Jan 23, 2023
Merged

Sentinel-2: use 10,000 as scale factor #1027

merged 9 commits into from
Jan 23, 2023

Conversation

osgeokr
Copy link
Contributor

@osgeokr osgeokr commented Jan 22, 2023

My suggestion is to apply a scale factor that converts DN to Reflectance of Sentinel-2 images.
Pls refer to this URL: https://docs.sentinel-hub.com/api/latest/data/sentinel-2-l2a/

Surface Reflectance values lies usually between 0.0 and 1.0. However, Specular effects on surface or clouds could lead to values higher than 1.0.
Pls refer to this document(p.40.): https://sentinel.esa.int/documents/247904/685211/Sentinel-2-MSI-L2A-Product-Format-Specifications.pdf

If the brightness adjustment is needed, it would be conceptually desirable to apply a seperate brightness factor. This is equivalent to adjusting the alpha value in image processing, regardless of the scale factor. Pls refer to this document: https://docs.opencv.org/3.4/d3/dc1/tutorial_basic_linear_transform.html

# image(reflectance) = image(DN) / 10000
bright = 5
adjusted_image = image * bright

The concept will be clearer if the brightness adjustment method is separated from the scale factor, such as torchvision.transforms.functional.adjust_brightness.

Thank you.

See #1025

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Jan 22, 2023
@adamjstewart
Copy link
Collaborator

Can you also fix docs/tutorials/custom_raster_dataset.ipynb? You don't have to open a new PR, you can just add another commit to the same branch and it will automatically show up.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 22, 2023
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.

I think we'll still want to clamp it to the 0–1 range (so keep the max=1). Otherwise, matplotlib may get confused whether the scale is supposed to be 0–1 or 0–255.

@adamjstewart adamjstewart added this to the 0.4.0 milestone Jan 22, 2023
@osgeokr
Copy link
Contributor Author

osgeokr commented Jan 22, 2023

@microsoft-github-policy-service agree

@adamjstewart adamjstewart changed the title Update sentinel.py Sentinel-2: use 10,000 as scale factor Jan 22, 2023
@osgeokr
Copy link
Contributor Author

osgeokr commented Jan 22, 2023

I agree. That seems like a wise choice.

@calebrob6
Copy link
Member

Thanks @osgeokr, do you mind calling .plot() on a Sentinel2 image with this change and posting what the result looks like?

adamjstewart
adamjstewart previously approved these changes Jan 23, 2023
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@osgeokr
Copy link
Contributor Author

osgeokr commented Jan 23, 2023

Thanks @osgeokr, do you mind calling .plot() on a Sentinel2 image with this change and posting what the result looks like?

In custom_raster_dataset.ipynb, the current scale factor is set to 6000. 6000 is assigned to the left figure and 10000 is assigned to the right figure. My opinion is to distinguish between scale factor and brightness factor in TorchGeo. Because this can be confusing to remote sensing majors like me. thank you.

SF

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.

10K looks better for those glaciers, but probably worse for agricultural land. But that's fine, we can always add an additional scale factor like you suggested. We should prob update the figure in the notebook though. Let me do that before I forget.

@adamjstewart
Copy link
Collaborator

I don't seem to have permission to push to your branch. Can you either enable commits from maintainers or re-run the notebook yourself to update the plots?

@calebrob6
Copy link
Member

Thanks @osgeokr! I agree.

@adamjstewart
Copy link
Collaborator

I'll just update the notebook in a follow-up PR. Thanks for the fix!

@adamjstewart adamjstewart merged commit ea18891 into microsoft:main Jan 23, 2023
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Update sentinel.py

* Delete custom_raster_dataset.ipynb

* Add files via upload

* Update sentinel.py

* Delete custom_raster_dataset.ipynb

* Add files via upload

* Update torchgeo/datasets/sentinel.py

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

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 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants