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

Update common band names table #22

Merged
merged 8 commits into from
Aug 1, 2024
Merged

Update common band names table #22

merged 8 commits into from
Aug 1, 2024

Conversation

m-mohr
Copy link
Contributor

@m-mohr m-mohr commented Jan 9, 2023

Implements #19

@m-mohr m-mohr linked an issue Jan 9, 2023 that may be closed by this pull request
Copy link
Member

@emmanuelmathot emmanuelmathot left a comment

Choose a reason for hiding this comment

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

This is a discrepancy regarding existing splitted CBN (e.g. nir08, nir09, swir16, swir22)
Either we keep splitting the common band with names based on their subranges or we use agnostic division number as in https://github.com/awesome-spectral-indices/awesome-spectral-indices#expressions.
The former is not ideal since all instruments do not share the same wavelengths division but has the advantage of a semantic information in the band name.
The latter is more flexible but we loose an information and in addition this would be a breaking change for future versions.

Personally, I'd rather keep the subrange information in the CBN.

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 9, 2023

So my use case is exactly "Awesome Spectral Indices". I'd like to apply a spectral index from the list to data and just get the correct assets (bands) from the metadata based on the common name (or wavelength, if no common name is present). This only works right now if the formula doesn't include a rededge1,2,3 band. The overlapping wavelengths (and CBNs) are an issue though.

@emmanuelmathot
Copy link
Member

emmanuelmathot commented Jan 11, 2023

I understand you requirement, I have the same since 2 years and I even needed new bands to be added in the CBN list. I pushed a PR at that time. Beyond the fact that the rule is that any new CBN must be shared by 3 or more other satellites, my comment here is more about the consistency of the nomenclature we adopt. Either a simple division number or an average wavelength number suffix (e.g. rededge70, rededge75).

@philvarner
Copy link

S2 red bands are:

  • B05 | Vegetation red edge, 704.1 nm (S2A), 703.8 nm (S2B) | 20m
  • B06 | Vegetation red edge, 740.5 nm (S2A), 739.1 nm (S2B) | 20m
  • B07 | Vegetation red edge, 782.8 nm (S2A), 779.7 nm (S2B) | 20m

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 30, 2023

Meeting notes: Needs a broader discussion, but rededge1-3 are not consistent with e.g. nir08 etc so we may want to change then to e.g. regedge70, rededge75 etc (as proposed by Emmanuel above). For this it would also be useful to know how other satellites do it and which bands they have. So I'll look that up and propose a name. Also we should check with ASI how they differentiate.

@davemlz
Copy link

davemlz commented Feb 3, 2023

Hey! Sorry to just jump into the discussion. @m-mohr opened and Issue in ASI (awesome-spectral-indices/awesome-spectral-indices#30) (I just renamed it), and it would be cool to have the mapping from the ASI standard to this standard. So, just to give you an overview, the bands and their standard in ASI were selected according to bands required by specific spectral indices (a band in a specific index shouldn't be ambiguous). E.g., in the case of Sentinel-2, there are indices that require the three red edge bands (see IRECI = (RE3 - R) / (RE1 / RE2)) -> This index couldn't be computed if the red edge bands were just a common unique band.

@m-mohr
Copy link
Contributor Author

m-mohr commented Feb 7, 2023

Interesting resource: https://eoreader.readthedocs.io/en/v0.19.1/optical_band_mapping.html

@m-mohr m-mohr changed the title Added rededge 1-3 as common names #19 Add additional rededge names as common names #19 Jul 9, 2024
@m-mohr
Copy link
Contributor Author

m-mohr commented Jul 30, 2024

@davemlz: What do you think about the current state? I've tried to find a compromise here that hopefully works for ASI and STAC equally well.

@m-mohr m-mohr changed the title Add additional rededge names as common names #19 Update common_names Jul 31, 2024
@m-mohr m-mohr changed the title Update common_names Update common names table Jul 31, 2024
@m-mohr m-mohr changed the title Update common names table Update common band names table Jul 31, 2024
@m-mohr m-mohr added this to the 2.0.0 milestone Jul 31, 2024
@emmanuelmathot emmanuelmathot mentioned this pull request Jul 31, 2024
@m-mohr
Copy link
Contributor Author

m-mohr commented Aug 1, 2024

Actually, I think we can merge this and update it during the beta if needed.

@m-mohr m-mohr merged commit 5cdfce6 into main Aug 1, 2024
2 checks passed
@m-mohr m-mohr deleted the rededge branch August 1, 2024 09:56
@davemlz
Copy link

davemlz commented Aug 2, 2024

Wow! Thank your for this!

@m-mohr this looks perfect. I will update the mapping from ASI to eo ;)

@davemlz
Copy link

davemlz commented Aug 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rededge1,2,3 to common names
4 participants