search

sertit / eoreader

Remote-sensing opensource python library reading optical and SAR sensors, loading and stacking bands, clouds, DEM and spectral indices in a sensor-agnostic way.
https://eoreader.readthedocs.io/en/latest/
Apache License 2.0
271 stars 22 forks source link

Fix quiclook not found for S2 L1C and L2A when not archived #84

Closed floriandeboissieu closed 1 year ago

floriandeboissieu commented 1 year ago

There are two bugs in the way it looks for the quicklook in the S2 .SAFE scene:

  1. the pattern was missing a wildcard
  2. The TCI is 10m resolution for L1C, thus better use PVI which is the preview image (although 320m resolution)

I did not change regex pattern for L2A products, although the PVI image also exists. I would recommend using it also instead of TCI_60m as this last one can be quite heavy (several MB). But I let you make that change if you feel like it is the good solution.

Your checklist for this pull request

Please review the guidelines for contributing to this repository.

Description

Please describe your pull request.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Thank you!

remi-braun commented 1 year ago

Thanks a lot for this contribution ! The quicklook section of EOReader is not the part the most tested 😅 So you did good proposing this PR !

To tell the truth, I wasn't even aware of the existence of the PVI, but you are right, this should be the quicklook instead of the PVI. You can make the change in your PR if you want, so you can be credited 😉

One question though. Some S2 products downloaded from AWS come with the preview.jpg file. You think those files should be the preview (although not georeferenced) or the PVI should be in any case ?

remi-braun commented 1 year ago

Second question, is it OK to have this in the future 0.20.0 version ? I don't really know when I will be releasing it, so if you want it asap, I could try to fix it in a patch version (0.19.4)

floriandeboissieu commented 1 year ago

No worries for the tests, it is always hard to cover everything (especially extras such as quicklook). Many thanks by the way for this very useful package. For your first question, I think it may depend on its use... A browser preview does not need to be georeferenced and a jpg file would be easier to read (according to S2 specs p. 461, it seems that it was the idea of the Browse Image, although I never tried/used this download option). However, it can also be useful to have a quicklook on a geolocated true color image, e.g. to check potential clouds on a specific area. But this could be done quite fast with COG. So I would say the best solution would be to have both :), i.e. preferentially preview.jpg as a preview image and PVI if it does not exist, while maybe referencing the PVI as an asset in case it is needed as georeferenced.

For the second question, I proposed it in 0.20 not to break your dev workflow, but if you feel it could belong in a 0.19.4 version (that could be make it available on conda) it would be great.

By the way, from what I have read in the above S2 specs, it is not clear for me if PVI is inherited from L1C (i.e. TOA) or recomputed from L2A TCI (i.e. BOA). So TCI 60m may be the best choice for L2A until it is clarified (I'll check when I have time and make another PR if necessary).

remi-braun commented 1 year ago

Hello,

Thanks for the feedback, this is really appreciated 😉 I have just create a 0.19.4 branch, so you can use it to update your PR :)

I agree to prioritize the .jpg file from AWS products.

However, in my opinion, we could always use PVI for L2A products. I'm not convinced the difference between BOA and TOA will be enough to be taken into account for such a dezoomed image. And if I understood it correctly, this is the way Sentinal-2 products are delivered, so I would follow their way and use the PVI as the quicklook.

floriandeboissieu commented 1 year ago

I have just create a 0.19.4 branch, so you can use it to update your PR :)

I changed the base branch from 0.20 to 0.19.4, solving the conflict by removing the differences. I hope I have done things well, please check.

However, in my opinion, we could always use PVI for L2A products. I'm not convinced the difference between BOA and TOA will be enough to be taken into account for such a dezoomed image. And if I understood it correctly, this is the way Sentinal-2 products are delivered, so I would follow their way and use the PVI as the quicklook.

I agree, I change the code to look for PVI for L2A product as well.

remi-braun commented 1 year ago

I changed the base branch from 0.20 to 0.19.4, solving the conflict by removing the differences. I hope I have done things well, please check.

Thanks for the effort! Unfortunately, I still see 58 files changed, so I'm wondering if you discarded the changes from the 0.20.0 (no mention of pixel_size should appear in this PR 😅 ) Could you just take a look ? (I am really not familiar with PR haha)

floriandeboissieu commented 1 year ago

That is what I was fearing changing the base branch, I am not very used to that kind of operation :sweat_smile: .

I close this PR and make another one directly on branch 0.19.4 .

remi-braun commented 1 year ago

Yeah it's safer haha Thanks for your perseverance!