Skip to content

Conversation

@lwhitefox
Copy link
Contributor

Running examples on the Pkg README using current versions of the pkgs that are included there causes failures when executing using PyramidScheme. The error msgs show incompatibilities in RasterMakieExt (stack trace below). I bumped versions of YAXArrays, YAXArrayBase and DimensionalData to current versions in the compat entries.
This also required fixes to the function Pyramid(path) to correctly call the appropriate Zarr or GDAL variants of the pyramid constructing functions. One set of tests in runtests.jl also needed an additional using DimensionalData to pass.
After these changes, I can locally run all CI tests successfully and execute the examples in the README without troubles.

Stack trace showing error when trying use current versions of the 3 pkgs listed above:

julia> Precompiling RastersMakieExt
        Info Given RastersMakieExt was explicitly requested, output will be shown live 
ERROR: LoadError: ArgumentError: invalid type for argument t in method definition for convert_arguments at /Users/USER/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/plotrecipes.jl:308
Stacktrace:
 [1] top-level scope
   @ ~/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/plotrecipes.jl:308
 [2] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [3] include(x::String)
   @ RastersMakieExt ~/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/RastersMakieExt.jl:1
 [4] top-level scope
   @ ~/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/RastersMakieExt.jl:12
 [5] include
   @ ./Base.jl:495 [inlined]
 [6] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2222
 [7] top-level scope
   @ stdin:3
in expression starting at /Users/USER/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/plotrecipes.jl:308
in expression starting at /Users/USER/.julia/packages/Rasters/0kAkQ/ext/RastersMakieExt/RastersMakieExt.jl:1
in expression starting at stdin:3
  ✗ Rasters → RastersMakieExt
  0 dependencies successfully precompiled in 6 seconds. 276 already precompiled.

@codecov
Copy link

codecov bot commented Jan 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.15%. Comparing base (19660b3) to head (733433f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
- Coverage   66.28%   66.15%   -0.13%     
==========================================
  Files           3        3              
  Lines         264      263       -1     
==========================================
- Hits          175      174       -1     
  Misses         89       89              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@felixcremer felixcremer left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.
I am not sure about the change to the loading. That seems to be unnecessarily convoluted.

function Pyramid(path::AbstractString, backend)
#This should rather be solved via dispatch, but this is not working because of Requires in YAXArrayBase.
if backend isa YAB.ZarrDataset
if (findfirst(x->x==:zarr,[keys(YAB.backendlist)...])!=nothing) && (typeof(backend) == YAB.backendfrompath("test.zarr"))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?
Shouldn't the backendtype from "test.zarr" always be a ZarrDataset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it is not ideal - the comment (not from me!) already shows there's some complications from YAXArrayBase. I don't know YAZArrayBase well, but the backend type returned from the backendfrompath is not easy to work with, which is why I set it up to avoid hard coding a type. But this may be just the limits of my knowledge of how the relevant pkgs set up types (and how to access them)

For example:

julia> using Zarr, YAXArrayBase

julia> tmp = YAXArrayBase.backendfrompath("test.zarr")
ZarrExt.ZarrDataset

julia> tmp <: ZarrDataset
ERROR: UndefVarError: `ZarrDataset` not defined
Stacktrace:
 [1] top-level scope
   @ REPL[23]:1

julia> tmp <: ZarrExt.ZarrDataset
ERROR: UndefVarError: `ZarrExt` not defined
Stacktrace:
 [1] top-level scope
   @ REPL[24]:1

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the problem is that the ZarrDataset type is defined in a package extension. I will have a look tomorrow how to improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pkg extensions are cool - but here it makes it hard to test the types, which creates challenges - if an extension is not loaded the code will error since the running code will not know about the :zarr backend, for example. That's why I put the first half of the test logic in (if (findfirst(x->x==:zarr,[keys(YAB.backendlist)...])!=nothing)) to make sure the :zarr backend is active before testing for a zarr file.

I'd be happy to have a simpler way to deal with it though!

@felixcremer
Copy link
Member

Thanks again for the PR. I incorporated the necessary changes into #56 and found a nicer way to deal with the YAXArrayBase.backendlist problem there.
Therefore I am going to close this but add you as a coauthor to the other PR.

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.

2 participants