Optimized SPADE analysis #419
Conversation
* Added cibuildwheel action * Added Python requirements to wheel build * Build only on 64bit machines, otherwise overflow * Removed Windows for testing, as vc is not available * Removed MacOS for testing, as -fopenmp is not available * Removed pp- (pypy) builds since they lack C. * Fixed removing pp- (pypy) builds since they lack C. * Put Macos back in. * Windows Hack * Remove vcpython alltogether, ignore 2.7 Python * Removed extra compile option, which breaks on Windows * Removed more extra compile options, which breaks on Windows * Try C++ instead of Gnu++. * Try C++ instead of Gnu++ Windows style argument. * Remove linux build while testing windows. * Remove libraries. * Differentiate Windows and Linux. * Added missing import. * Last mile: MacOS * Remove openMP lib * Remove openMP lib * Add openMP lib * More brew installs * Mac is called mac on github * Make sure C is reinstalled. * Multilib * Next try, new options * Ignore warning about void type * Update newsest fim package * Revert "Ignore warning about void type" This reverts commit 3ff6b62 * Revert to prior fim, new compiler argument. * Revert "Update newsest fim package" This reverts commit f321f77 * Definitely, gnu++17, but new try. * Try C++ * Warning message * llvm maybe? * Added apple in source * Small fixes for MacOS, but not comprehensive * Limit to Windows and Linux for now * Remove MacOS entry * Fix fix from mindlessness * Testrun * Trying to include fim.so, despite its renaming by wheels * Added newest version of original module * Reverted previous breaking change commited by accident. * Reverted package name from testing. * Test focal as CI build * Test bionic as CI build * Understand installation issue on CI -- is importing elephant importing the installed version? * Spelling error only * Try to make sure travis loads the installed elephant, not the cwd. * One step further -- which version will nosetests use? * Switch to pytest as of PR NeuralEnsemble#413
|
Hello @mdenker! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-08-13 06:01:30 UTC |
|
@mdenker I commented out the test as a filter for the maximum number of occurrences is not implemented in Florian's fpgrowth. It is a filtering not to mine patterns with more occurrences of the one fixed. I think we can just remove it from SPADE, as it is an unintuitive parameter given the purposes of the method. I don't see why one should decide to mine patterns with fewer occurrences that the ones that can be found... |
|
@stellalessandra Ah, thanks for the clarification. Then I suggest to leave the unit test commented for now, and discuss removing this parameter in a new issue/PR, in order not to blow up this PR. |
…accelerated_spade # Conflicts: # MANIFEST.in
elephant/test/test_spade.py
Outdated
| # def test_different_surrogate_method(self): | ||
| # np.random.seed(0) | ||
| # random.seed(0) | ||
| # spiketrains = [stg.homogeneous_poisson_process(rate=20*pq.Hz) | ||
| # for _ in range(2)] | ||
| # surr_methods = ('dither_spikes', 'joint_isi_dithering', | ||
| # 'bin_shuffling', | ||
| # 'dither_spikes_with_refractory_period') | ||
| # pv_specs = {'dither_spikes': [[2, 2, 0.8], [2, 3, 0.2]], | ||
| # 'joint_isi_dithering': [[2, 2, 0.8]], | ||
| # 'bin_shuffling': [[2, 2, 1.0], [2, 3, 0.2]], | ||
| # 'dither_spikes_with_refractory_period': | ||
| # [[2, 2, 0.8]]} | ||
| # for surr_method in surr_methods: | ||
| # pv_spec = spade.pvalue_spectrum( | ||
| # spiketrains, bin_size=self.bin_size, | ||
| # winlen=self.winlen, dither=15*pq.ms, | ||
| # n_surr=5, surr_method=surr_method) | ||
| # self.assertEqual(pv_spec, pv_specs[surr_method]) |
There was a problem hiding this comment.
What's up with this test?
There was a problem hiding this comment.
Not sure -- it does pass for me, also appear to be functional. @stellalessandra Do you know why this one is commented out?
There was a problem hiding this comment.
I think we should remove it. It is a test that cross-checks across some surrogate methods if they lead to the same p-value spectrum. But, it is not general enough, and we have tests in spike_train_surrogates.py that check the correctness of the surrogate techniques...
There was a problem hiding this comment.
Ok, I see the point, I'd agree this would be somewhat redundant, and may break in case of changes to the implementations of the surrogate methods, even if those are correct (i.e.: maybe after an update of the algorithm, a surrogate method produces a different realization with a given random seed -- in that case this test would start to fail, but there would be no bug.
Co-authored-by: Alexander Kleinjohann <[email protected]>
Co-authored-by: Alexander Kleinjohann <[email protected]>
… of surrogate methods
This PR includes a new version of the FP-growth algorithm for the SPADE analysis based on the work of Florian Porrmann (https://github.com/fporrmann/FPG). The new library features a number of SPADE specific optimizations that improve run-time significantly.
In order to include the C++ module, the install procedure was altered to compile the corresponding
fim.sofrom scratch, rather than downloading a precompiled module post-install. Also, recipes for building pip wheels for Linux and Windows were included as github actions, building incibuildwheels. Currently, these are configured to run on every push, and after a testing period, this may be altered to build only after a PR or even only after a version-tagged PR.Also, a change from
nosetesttopytestwas required in order to have available the--import-liboption that makes sure the compiled version of the library is used for unit testing. This includes most changes of #413 .@stellalessandra There is one unit test in line 275 of
test_spade.pythat you had commented out, and which fails. Does this need fixing?