-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix quiclook not found for S2 L1C and L2A when not archived #84
Conversation
…d confusion about the definitions (especially for SAR data) sertit#82
- DEPS: Pin sertit to 1.25.0
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.
Thanks a lot for this contribution ! 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. 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 ? |
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) |
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 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). |
Hello, Thanks for the feedback, this is really appreciated 😉 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. |
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.
I agree, I change the code to look for PVI for L2A product as well. |
Thanks for the effort! |
That is what I was fearing changing the base branch, I am not very used to that kind of operation 😅 . I close this PR and make another one directly on branch 0.19.4 . |
See sertit#84 for details
Yeah it's safer haha |
…TCI file if no .jpg preview file is found (#84 #85, thanks a lot @floriandeboissieu)
See sertit#84 for details
…TCI file if no .jpg preview file is found (sertit#84 sertit#85, thanks a lot @floriandeboissieu)
There are two bugs in the way it looks for the quicklook in the S2 .SAFE scene:
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!