Correct inverse plan logic

[gaurav-arya]

This PR fixes some incorrect caching logic in the test plans. The reason this escaped notice is because ScaledPlan uses plan_inv rather than inv. That has been changed too.

(Old Description)

There is no need to do the caching in plan_inv as it should be handled by AbstractFFTs. The current logic leads to the following error which is now fixed.

julia> using AbstractFFTs
[ Info: Precompiling AbstractFFTs [621f4979-c628-5d54-868e-fcf4e3e8185c]

julia> using AbstractFFTs: Plan

julia> include("test/testplans.jl")

julia> p = plan_fft(rand(3))
TestPlan{ComplexF64, 1}(1:1, (3,), #undef)

julia> inv(inv(p).p)
ERROR: TypeError: in typeassert, expected AbstractFFTs.ScaledPlan{ComplexF64, TestPlan{ComplexF64, 1}, ComplexF64}, got a value of type TestPlan{ComplexF64, 1}
Stacktrace:
 [1] inv(p::InverseTestPlan{ComplexF64, 1})
   @ AbstractFFTs ~/repos/AbstractFFTs.jl/src/definitions.jl:237
 [2] top-level scope
   @ REPL[5]:1

[codecov]

Codecov Report

Merging #69 (8d7adec) into master (3e7d412) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   83.09%   83.09%           
=======================================
  Files           2        2           
  Lines         207      207           
=======================================
  Hits          172      172           
  Misses         35       35           

Impacted Files	Coverage Δ
src/definitions.jl	66.01% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e7d412...8d7adec. Read the comment docs.

[devmotion]

as it should be handled by AbstractFFTs

Can you explain this?

[gaurav-arya]

It seems like the inv function in AbstractFFTs (lines 237-238 in the main branch currently) wraps plan_inv and takes care of the caching in pinv automatically, so plan_inv should be a pure function that just constructs the inverse plan

[gaurav-arya]

Ah, I see now that those lines were pre-caching pinv for the inverse plan, not the original plan.

Nevertheless, given how the caching is implemented in AbstractFFTs, I don't think we are allowed to do that (as evidenced by the error).

Fundamentlaly, the issue is that plan_inv is expected to output the same type as the pinv field, because of that hack being done in pinv_type. Whereas here the pinv field of the inverse is cleverly storing the original plan, but the plan_inv function is returning a scaled plan, and that doesn't match.

We can either remove the caching or edit the plan_inv returned value to use the cached value (I.e. the original plan). The first option seemed simplest and matches what is done in FFTW I think.

[devmotion]

In

AbstractFFTs.jl/src/definitions.jl

Lines 237 to 238 in 3e7d412

	inv(p::Plan) =
	isdefined(p, :pinv) ? p.pinv::pinv_type(p) : (p.pinv = plan_inv(p))

the inverse plan is computed if it doesn't exist yet. And in the tests we try to avoid that since we already now the inverse plan. So by removing the lines there will be additional computations it seems?

I think the whole caching approach is a bit suboptimal as it's not done in a type stable way and hence requires hacks such as the type annotation in 238 it seems. However, if we really think that this is the only valid type for the inverse plan then inv seems the wrong place to declare it. IMO it would be much more reasonable to change the structs to something like

struct Plan{...,P}
    ...
    pinv::Union{Nothing,P}
end

and compute type parameter P in the constructor with pinv_type. Then methods such as inv would only check p.pinv === nothing ? (p.pinv = plan_inv(p)) : p.pinv, without any type annotations. The compiler is optimized for such unions with Nothing, and if it's really needed one can always resort to using the type parameter P explicitly.

Additionally, I think it would be reasonable for plan_inv(p) to prepopulate the pinv field of the inverse plan automatically since p is already provided. Recomputing an inverse plan for the inverse at some point seems a bit unnecessary. So I think ideally plan_inv would take care of setting pinv and it would not be required to that manually in the tests.

[gaurav-arya]

I agree that ideally that hack shouldn't be necessary. But shouldn't that be left to a separate PR? i.e. this discussion seems to be more appropriate for #20

All this PR is doing is getting the test plans to conform to whatever the current design of AbstractFFTs expects plans to do: namely, have a pinv field but do not do anything with it, as this is required for tests to pass in #67.

[devmotion]

All this PR is doing is getting the test plans to conform to whatever the current design of AbstractFFTs expects plans to do: namely, have a pinv field but do not do anything with it, as this is required for tests to pass in #67.

I don't see where the API and implementation of AbstractFFTs disallows setting the inverse of the inverse in plan_inv. Even more so since the field for the inverse is not concretely typed, so there are no issues with type parameters either. Therefore I think that even with the current, probably suboptimal design it should not be necessary to remove it from the tests. I assume the issue is not that pinv is set but rather that it is set incorrectly - as it does not seem to confirm with the pinv_type that AbstractFFT expects. Probably related to the scaling since that would also explain why it expects a ScaledPlan and complains that it only got an TestPlan.

[gaurav-arya]

Okay. Tonight (or time zone agnostically, in 10 hours) I'll take a stab at modifying things so that pinv_type works, but without removing the caching.

[gaurav-arya]

As you suggested, there was a correctness issue with the plan caching, and fixing that without removing the caching works. I've updated the PR description accordingly.

[devmotion]

So to understand this correctly: that was a bug in the package and when fixing it tests would fail currently (without the changes in test/testplans.jl)?

[gaurav-arya]

That's right: changing this without making changes to the test plans caused tests to fail.

I'm not sure to what extent I'd call the original version a bug in the package, but it did lead to ignoring the pinv field of any plan inside a scaled plan: so for example given a plan P a new inverse plan would be made for 2 * P, 3 * P, 4 * P, etc., which is why the caches made by the test plans were just ignored.

[devmotion]

OK. I just wanted to make sure that this change is covered by the tests, ie that reverting it would cause errors.

[devmotion]

LGTM, thank you!