search

meggart / DiskArrays.jl

Other
73 stars 13 forks source link

fix macros #92

Closed rafaqz closed 1 year ago

rafaqz commented 1 year ago

This PR fixes the escaping of all the macros so they work outside this package, and tests that macro application works.

I also had to define a few more macros for where AbstractDiskArray was used in dispatch - for views and batch indexing.

@meggart I switch the test types to be from the the macro rather than inheriting from AbstractDiskArray.

But there is this one broken test I don't understand:

@test_broken getindex_count(a) == 3

I had to change AbstractUnitRange to OrdinalRange in the test methods once I used the macros.

Any Ideas?

rafaqz commented 1 year ago

@tcarion you can probably use this branch for testing already, it's one minor test failing currently

meggart commented 1 year ago

Thanks a lot for fixing this. I had a look already on the weekend and it looks fine so far. The failing tests came from a more fundamental issue: What exactly should be the range types an AbstractDiskArray subtype should support. In the beginning I thought a DiskArray must only implement

readblock!(a,aout,r::AbstractUnitRange...)

i.e. the backend only implements reading dense blocks of data and other accesses are handled by DiskArrays.jl. However, because packages like NetCDF have optimised methods for reading e.g. every second value of a dimension, we started to hand over these types of accesses to the backend as well. We then had some fallbacks for AbstractDiskArrays, but these obviously did not work for the macro-style implementation.

I will make an issue about this, where we maybe can re-consolidate what the actual interface for DiskArrays is and then reconcile the unit tests so that they reflect exactly the interface that is documented.

rafaqz commented 1 year ago

Yes that error was from me not understanding the dispatch mechanism, and also accidentally defining new readblock methods rather than DiskArrays.readblock in the macros.

But it would be great to make the interface really clear.

Also apologies for the merge without review, I was concerned about blocking the work of converting NCDatasets and GRIBdatasets to DiskArrays, as they have a lot of momentum right now.

meggart commented 1 year ago

No problem about the merge, I know that I am not really reliable in doing reviews in time, so best to not block the work of others. I don't think any dependent package is broken right now, but we should extend the unit tests and make sure to test both the behavior for AbstractDiskArrays as well as Arrays that just implement the macros + arrays that have methods for StepRanges + arrays that don't.