MagneticResonanceImaging / MRIReco.jl

Julia Package for MRI Reconstruction
https://magneticresonanceimaging.github.io/MRIReco.jl/latest/
Other
85 stars 22 forks source link

CompatHelper: bump compat for LinearOperators to 2, (keep existing compat) #81

Closed github-actions[bot] closed 2 years ago

github-actions[bot] commented 2 years ago

This pull request changes the compat entry for the LinearOperators package from = 2.2.3 to = 2.2.3, 2. This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.

alexjaffray commented 2 years ago

@tknopp the cause of the CI run failing is related to line 168 in LinearOperators.jl/src/abstract.jl.

The function definition: storage_type(op::LinearOperator) = typeof(op.Mv5)

should be: storage_type(op::AbstractLinearOperator) = typeof(op.Mv5)

Changing this line in my local fork of LinearOperators seems to alleviate the issue. (but obviously doesn't alleviate the other interface issues), while passing tests for the LinearOperators.jl package. I will make a PR to LinearOperators with the change from my local fork and see if it was an oversight in going from 2.2 -> 2.3 in LinearOperators, or if it's a design choice to force storage type implementation as part of the interface.

tknopp commented 2 years ago

@alexjaffray: Thanks. I have not looked deeper into this but it seems that @migrosser also detected the issue and added these definitions:

https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/176bab44b5da45521db6f3faa553b657ccae5d33

What is the path forward? uncommenting these definitions and relying on 2.3?

(but obviously doesn't alleviate the other interface issues)

are there others? Actually they would need to bump to 3.0 according to semantic versioning...

alexjaffray commented 2 years ago

There's a discussion in LinearOperators that will hopefully define the way forward for us. Once that's been determined, we can decide what to do here.

alexjaffray commented 2 years ago

@migrosser and @tknopp the discussion with LinearOperators people was resolved. The way forward for now is that we need to implement the storage_type methods for MRIReco's linear operators on our end. For any type that has no implementation, the fallback method in LinearOperators will now provide a meaningful error message, instructing that a storage_type method must be defined.

I guess now we just have to define the methods, so we should try uncommenting the definitions @migrosser introduced and seeing if CI passes with LinearOperators v2.3.x as done in https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/176bab44b5da45521db6f3faa553b657ccae5d33

tknopp commented 2 years ago

fixed in https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/c8bf1e4448ccbe4320b12833a2db3653d13cb46f