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

Make export pipeline logs more readable #111

Merged
merged 5 commits into from Oct 19, 2022
Merged

Conversation

zigaLuksic
Copy link
Collaborator

Silences output of gdal calls in favor of tqdm, making logs much more readable.

In the logs there was a constant warning:

Warning 1: General options of gdal_translate make the COPY_SRC_OVERVIEWS creation option ineffective as they hide the overviews

I have removed this option in this MR, but it should be investigated if that is really the way to go. Link to cogification docs

Copy link
Contributor

@mlubej mlubej left a comment

Choose a reason for hiding this comment

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

Not much to review, the changes make sense to me. One question, should this be parametrizable via the pipeline? I guess not, but just wanted to point it out anyway.

@zigaLuksic
Copy link
Collaborator Author

Not much to review, the changes make sense to me. One question, should this be parametrizable via the pipeline? I guess not, but just wanted to point it out anyway.

i was thinking about it, but I didn't see much point in keeping the old logs. Also not having them silenced breaks the tqdm

@zigaLuksic
Copy link
Collaborator Author

I talked to the GDAL wizards and they informed me that we are idiots (which we knew) and are using a cogification method for old GDAL and/or planar tiffs, which are not suitable for us (which we didn't know).

So i switched to the recommended way of cogification for newer GDAL versions.

@mlubej
Copy link
Contributor

mlubej commented Oct 19, 2022

So i switched to the recommended way of cogification for newer GDAL versions.

Did you check if the GDAL version used is the appropriate one? Should be GDAL 3.1 or newer.

@mlubej
Copy link
Contributor

mlubej commented Oct 19, 2022

I remembered about one more potential issue. if you're using deflate compression with Float32 values, you should set the predictor to 3

from docs:

NOTE: for many types of data adding a predictor can further reduce the file size. It is best you test this on your own data. To enable the predictor, add to the above command -co PREDICTOR=2 for integers, and -co PREDICTOR=3 for floating points.

@zigaLuksic
Copy link
Collaborator Author

So i switched to the recommended way of cogification for newer GDAL versions.

Did you check if the GDAL version used is the appropriate one? Should be GDAL 3.1 or newer.

GDAL 3.1 was released in 2020. Do you think we need to check the version and raise an exception if the GDAL version is older than that?

I remembered about one more potential issue. if you're using deflate compression with Float32 values, you should set the predictor to 3

Hmmm, at this point perhaps the utility functions should have a is_discrete flag which then sets these parameters accordingly 🤔

@mlubej
Copy link
Contributor

mlubej commented Oct 19, 2022

GDAL 3.1 was released in 2020. Do you think we need to check the version and raise an exception if the GDAL version is older than that?

It might make sense, because older versions of GDAL had some mismatch issues where the resulting tiff could contain an offset. Not sure how relevant it is due to it being released in 2020, but I imagine it could happen..

Some more info I remember from https://git.sinergise.com/sentinel-core/java/-/issues/1400

Hmmm, at this point perhaps the utility functions should have a is_discrete flag which then sets these parameters accordingly 🤔

What if we use the dtype parameter for this, since it's limited to ["int8", "int16", "uint8", "uint16", "float32"]? This way if the user provides float32 (or if float32 is recognized automatically), then the predictor 3 would be used. I would assume if the inputs are 1.0, 2.0, 3.0, ... the user would provide dtype int8 use integers.

@zigaLuksic
Copy link
Collaborator Author

What if we use the dtype parameter for this, since it's limited to ["int8", "int16", "uint8", "uint16", "float32"]? This way if the user provides float32 (or if float32 is recognized automatically), then the predictor 3 would be used. I would assume if the inputs are 1.0, 2.0, 3.0, ... the user would provide dtype int8 use integers.

how in the absolute hell did i miss that i have dtype at my disposal....

@zigaLuksic zigaLuksic merged commit 2210bff into develop Oct 19, 2022
@zigaLuksic zigaLuksic deleted the export-pipeline-logs branch October 19, 2022 09:58
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.

None yet

2 participants