Skip to content

Conversation

@tebadi
Copy link
Contributor

@tebadi tebadi commented Jun 21, 2022

Currently the GeoMAD SR band names differ from Collection 3 NBART product. The issue was reported by Robbie :

image

where left is C3 Landsat band names and right is the current GeoMAD.

This fix is addressing the inconsistency issue.

@tebadi tebadi requested review from emmaai and robbibt June 21, 2022 07:43
@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #51 (7ed06af) into develop (3d014da) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##           develop      #51   +/-   ##
========================================
  Coverage    62.41%   62.41%           
========================================
  Files           21       21           
  Lines         1998     1998           
========================================
  Hits          1247     1247           
  Misses         751      751           
Impacted Files Coverage Δ
odc/stats/plugins/gm.py 65.78% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d014da...7ed06af. Read the comment docs.

Copy link

@robbibt robbibt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @tebadi! I imagine the band aliases themselves can be updated somewhere downstream/not in stats?
image (in case we wanted to add the old names as aliases to match C3 Landsat)

@tebadi
Copy link
Contributor Author

tebadi commented Jun 21, 2022

Yes, that will be changed in the product definitions Robbi.

@emmaai
Copy link
Contributor

emmaai commented Jun 22, 2022

I don't think we need do this? We can just add aliases in the product definition as ARD products have been doing.

@tebadi
Copy link
Contributor Author

tebadi commented Jun 22, 2022

I don't think we need do this? We can just add aliases in the product definition as ARD products have been doing.

This is fixing it at the source instead of fixing it downstream.

Copy link
Contributor

@emmaai emmaai left a comment

Choose a reason for hiding this comment

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

If that’s the argument, then the input bands name should be fixed rather than hard mapping here. It was to take input band name as the output band name. The reason that we have seemingly different band names than what you preferred is because it loaded the bands by one of many aliases provided by ard. If it only loads the other aliases you like, it’ll give you the preferred band names.

Besides, this hard mapping is so fragile, slightly change on inputs bands name will render the whole thing broken, the same example that the input bands aliases change.

@tebadi
Copy link
Contributor Author

tebadi commented Jun 22, 2022

If that’s the argument, then the input bands name should be fixed rather than hard mapping here. It was to take input band name as the output band name. The reason that we have seemingly different band names than what you preferred is because it loaded the bands by one of many aliases provided by ard. If it only loads the other aliases you like, it’ll give you the preferred band names.

I'd like fixing the band names to the preferred/output band names as opposed to adding alias here. It's more readable and achieves what we need for consistency between ARD and GeoMAD spectral bands. Done.

@tebadi tebadi merged commit 5479f91 into develop Jun 22, 2022
@tebadi tebadi deleted the hot_fix branch June 22, 2022 06:05
emmaai pushed a commit that referenced this pull request Sep 30, 2022
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.

4 participants