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

RFC: Allow to use interactive Datasets in unsafe_gdal functions in utilities.jl #167

Closed
wants to merge 4 commits into from

Conversation

felixcremer
Copy link
Contributor

As suggested by @visr in #164 this widens the expected types in the unsafe_gdal* functions to AbstractDataset, so that they would be usable with interactive Datasets.
I am using parametric functions, so that we reuse the input type as return type to prevent the accidental mixing of the different approaches.

I added tests for these functions by copying the tests in test_gdalutilities.jl and change the do block notation to variable assignment.

Should we also provide these functions without the unsafe keyword by defining:

gdalfunc(IDataset, options; kwargs) = unsafe_gdalfunc(IDataset, options; kwargs)

This would allow to build longer input arrays into vrts.
This allows to use interactive datasets in the unsafe_gdal functions in
utilities.jl.
Allow subtypes of AbstractDataset in unsafe_gdal... functions so that we
could use these with IDatasets. We use the type of the input as return
type, so that we wouldn't accidentally change the approach by using
these functions.
The tests are copied from the non-interactive utilities tests and
converted to not use the do block notation, but to set the variable and
reuse the variable later in the tests.
@coveralls
Copy link

coveralls commented Jan 22, 2021

Coverage Status

Coverage remained the same at 93.329% when pulling 3e6c9ff on felixcremer:unsafevrt into 40c7a2b on yeesian:master.

@visr
Copy link
Collaborator

visr commented Jan 22, 2021

Great, thanks!

Should we also provide these functions without the unsafe keyword by defining:

This should already be done automatically. Try leaving out unsafe_ in the extra tests you added. Note also #142, because these functions have some discoverability issues that would be good to resolve.

Copy link
Owner

@yeesian yeesian 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 great, thanks! My comments here are just echoing what @visr has said in #167 (comment), in case having it might help.


If you want corresponding functions that returns the Interactive versions of the corresponding datasets, you can still do so but it has to be a separate implementation. E.g. for gdalwarp, adding the following lines:

function gdalwarp(
        datasets::Vector{T},
        options = String[];
        dest = "/vsimem/tmp"
    ) where T<:AbstractDataset
    options = GDAL.gdalwarpappoptionsnew(options, C_NULL)
    usage_error = Ref{Cint}()
    result = GDAL.gdalwarp(dest, C_NULL,
        length(datasets), [ds.ptr for ds in datasets], options, usage_error)
    GDAL.gdalwarpappoptionsfree(options)
    return IDataset(result)
end

If an example might help, see e.g. how it's done for fromWKB.

options = GDAL.gdaltranslateoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdaltranslate(dest, dataset.ptr, options, usage_error)
GDAL.gdaltranslateoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalwarpappoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalwarp(dest, C_NULL,
length(datasets), [ds.ptr for ds in datasets], options, usage_error)
GDAL.gdalwarpappoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalvectortranslateoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalvectortranslate(dest, C_NULL, length(datasets),
[ds.ptr for ds in datasets], options, usage_error)
GDAL.gdalvectortranslateoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

@@ -129,11 +129,11 @@ function unsafe_gdaldem(
result = GDAL.gdaldemprocessing(dest, dataset.ptr, processing, colorfile,
options, usage_error)
GDAL.gdaldemprocessingoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalnearblackoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalnearblack(dest, C_NULL, dataset.ptr, options, usage_error)
GDAL.gdalnearblackoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalgridoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalgrid(dest, dataset.ptr, options, usage_error)
GDAL.gdalgridoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalrasterizeoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalrasterize(dest, C_NULL, dataset.ptr, options, usage_error)
GDAL.gdalrasterizeoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

options = GDAL.gdalbuildvrtoptionsnew(options, C_NULL)
usage_error = Ref{Cint}()
result = GDAL.gdalbuildvrt(dest, length(datasets),
[ds.ptr for ds in datasets], C_NULL, options, usage_error)
GDAL.gdalbuildvrtoptionsfree(options)
return Dataset(result)
return T(result)
Copy link
Owner

Choose a reason for hiding this comment

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

This should still be Dataset(result) rather than T(result) (see #167 (comment) for context).

felixcremer added a commit to meggart/DeepCubeGen that referenced this pull request Apr 27, 2021
This adds the Project.toml and the pluto notebook to explain how we can
load data in the ESDL and save it as zarr or netcdf format.
This currently depends on
yeesian/ArchGDAL.jl#167 for the use of
unsafe_buildvrt.
@yeesian
Copy link
Owner

yeesian commented Jun 24, 2021

This has been incorporated in #179, thank you for the contribution!

@yeesian yeesian closed this Jun 24, 2021
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