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

Add integration test with FFTW backend #75

Closed
wants to merge 1 commit into from

Conversation

gaurav-arya
Copy link
Contributor

@gaurav-arya gaurav-arya commented Aug 17, 2022

While thinking about how to test JuliaMath/FFTW.jl#249, I thought it might be nice to just run the test suite here with FFTW. Integration tests would also be useful for sanity checking that the chain rules, etc. do correctly work with FFTW, so it seems like a good thing to have in general.

The PR:

  • Puts the test plans into a module to make clear that it is meant to act similarly to FFTW
  • Separates the FFT testing functions into their own function testfft. Ideally, testfft would take the FFT backend as input, but see the note in the file :/ At least it makes clear what the issue is currently (see Multiple FFT backends? #32). Hopefully we can add a backend parameter to plan_fft in a future PR (and let FFTW be the default backend for backwards compat)
  • Adds FFTW and TestPlans groups for testing

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #75 (6923730) into master (03ef58b) will decrease coverage by 0.48%.
The diff coverage is n/a.

❗ Current head 6923730 differs from pull request most recent head 4347fa9. Consider uploading reports for the commit 4347fa9 to get more accurate results

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   83.09%   82.60%   -0.49%     
==========================================
  Files           2        2              
  Lines         207      207              
==========================================
- Hits          172      171       -1     
- Misses         35       36       +1     
Impacted Files Coverage Δ
src/definitions.jl 65.04% <0.00%> (-0.98%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gaurav-arya gaurav-arya changed the title Run test set with FFTW backend too Add integration test with FFTW backend Aug 18, 2022
@gaurav-arya
Copy link
Contributor Author

gaurav-arya commented Aug 18, 2022

Unfortunately, there's no version of FFTW compatible with Julia version 1.0 and AbstractFFTs version 1. So I don't see a way to add FFTW as an extra in the Project.toml while still supporting Julia 1.0, unless I'm missing something: even if I exclude the FFTW integration test on Julia 1.0, it's still in the Project.toml, so the package tests error on Julia 1.0.

Is it reasonable to bump the minimum Julia compat to 1.3 (minimum that I think should work) or even 1.6 (LTS and FFTW's current compat) in order to get the integration test to work? Or better to forget about integration tests (downstream packages like FFTW can verify adjoints, chain rules themselves to the extent desired. The tests here can still verify it for TestPlans, which gives pretty good confidence of correctness). @stevengj @devmotion do you have any thoughts?

@devmotion
Copy link
Member

I think it would be good to add downstream tests of e.g. FFTW but IMO it should be done as in ChainRules, SciML, Turing, StatsFuns, etc. by running the tests of the downstream package in a separate action. The main problem with this PR is that it adds a circular dependency which IMO is very annoying to work with and causes test failures every time you introduce a breaking change (e.g. because the downstream package can't be installed since it is not compatible yet). I'll open a PR with the downstream testing setup of these other packages.

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