Skip to content

Implement AxographIO module with tests#550

Merged
JuliaSprenger merged 2 commits intoNeuralEnsemble:masterfrom
will-hart:axograph_rawio
Sep 18, 2018
Merged

Implement AxographIO module with tests#550
JuliaSprenger merged 2 commits intoNeuralEnsemble:masterfrom
will-hart:axograph_rawio

Conversation

@will-hart
Copy link
Contributor

This PR proposes an IO class for reading from AXGX and AXGD files using axographio. It provides an alternative to StimfitIO for importing axograph files which may be a bit easier to get up and running, particularly on Windows.

@pep8speaks
Copy link

pep8speaks commented Jul 12, 2018

Hello @will-hart! Thanks for updating the PR.

Line 152:19: W292 no newline at end of file

Comment last updated on September 17, 2018 at 22:08 Hours UTC

@coveralls
Copy link

coveralls commented Jul 12, 2018

Coverage Status

Coverage increased (+0.007%) to 49.054% when pulling 020ef0b on will-hart:axograph_rawio into 1e4b4c6 on NeuralEnsemble:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.7%) to 45.513% when pulling d3a7d89 on will-hart:axograph_rawio into 42f6649 on NeuralEnsemble:master.

@samuelgarcia
Copy link
Contributor

@will-hart: thank you very much for this PR.

  • Could you add the testing file you have in https://web.gin.g-node.org at the good place ?
  • axographio python module seems to be easy to install with pip, so maybe we could add this module in cicle-ci so a full test of this module would be done.

@will-hart
Copy link
Contributor Author

Yes, I've had some trouble signing up to gin as my University account thinks the email is spam and blocks it for more than 3 hours, meaning the activation link expires :) I'll upload the file as soon as I can make an account. I'll also sort out the PEP8 fixes.

I'm not sure about the code coverage reducing - have I messed up the tests somehow?

@cgars
Copy link

cgars commented Jul 12, 2018

@will-hart I took the liberty to manually activate your GIN account. Feel free to sign in.

Cheers

@will-hart
Copy link
Contributor Author

@JuliaSprenger
Copy link
Member

The axographio tests are currently not running on circle ci due to the missing axograph installation. This PR https://github.com/will-hart/python-neo/pull/1 should fix this. @will-hart: can you give it a try?

@JuliaSprenger JuliaSprenger added this to the 0.7.0 milestone Jul 26, 2018
@jpgill86
Copy link
Contributor

jpgill86 commented Aug 2, 2018

As the current maintainer of axographio, I'm excited by this and am eager to see it in the next release of Neo.

Currently two tests are failing for both ci/circleci: test-2.7 and ci/circleci: test-3.6:

  • FAIL: test_IrregularlySampledSignal_repr (neo.test.coretest.test_irregularysampledsignal.TestIrregularlySampledSignalProperties)
  • FAIL: test__repr (neo.test.coretest.test_spiketrain.TestPropertiesMethods)

I'd like to help, but it's not clear to me that either of these relate to neo.io.AxographIO. Is the issue elsewhere?

@JuliaSprenger
Copy link
Member

Hi @jpgill86, I agree it's a bit weird these seemingly unrelated tests fail by adding axographio to the requirements. I suspect there are additional changes in other package versions coming with it and thus failing these tests, but I haven't had time to look into this. It's on my todo list, but I can't promise anything.

@jpgill86
Copy link
Contributor

jpgill86 commented Aug 2, 2018

I discovered a couple 3 bugs in the AxographIO implementation and have opened a pull request with fixes: will-hart/python-neo#2. @will-hart, can you verify and merge?

@will-hart
Copy link
Contributor Author

Done, thanks for taking a look at the PR!

@jpgill86
Copy link
Contributor

I think PR will-hart#3 should resolve these failing tests. Looking forward to seeing AxographIO in a Neo release!

@jpgill86
Copy link
Contributor

Great, ci/circleci: test-2.7 and ci/circleci: test-3.6 are passing when once they were failing, but now continuous-integration/travis-ci/pr is failing when once it was passing!

The problem seems to be isolated to the Python 3.5 build in TravisCI. I can't imagine what might be going on, but I'll take another look.

@jpgill86
Copy link
Contributor

jpgill86 commented Aug 16, 2018

The issue appears to depend on which version of NumPy is installed and doesn't actually depend on the Python version. I've only done some spot testing, but it seems that the older NumPy 1.14.5 does not fail these tests, but the newer NumPy 1.15.0 does. This is true even if I install Neo from upstream master instead of from this topic branch, so it seems to have nothing to do with axographio.

Here's a minimal reproduction of one of the tests currently failing for this PR:

import neo
import numpy as np
import quantities as pq
from numpy.testing import assert_array_equal
from neo.test.coretest import test_analogsignal

# the following is essentially a reproduction of a failing test:
#   TestAnalogSignalArrayMethods.test_splice_1channel_inplace

t = test_analogsignal.TestAnalogSignalArrayMethods()

t.setUp()

signal_for_splicing = neo.AnalogSignal(
    [0.1, 0.1, 0.1],
    t_start=3 * pq.ms,
    sampling_rate=t.signal1.sampling_rate,
    units=pq.uA)
result = t.signal1.splice(signal_for_splicing, copy=False)
assert_array_equal(t.signal1, result)

On my machine, if I create fresh environments in Anaconda and install NumPy 1.14.5 and Neo from upstream master, this code runs fine. If NumPy 1.15.0 is install, the assertion fails. This is the case regardless of whether the Anaconda environment is Python 3.6, 3.5, or 2.7.

For some reason that I haven't been able to discover yet, the older NumPy version 1.14.5 was selected and installed automatically when I used pip install numpy on my machine in the fresh Python 3.6 and 2.7 environments. When the same command was used in a fresh Python 3.5 environment, the newer NumPy version 1.15.0 was selected and installed. This would be consistent with the TravisCI results, since it would make tests fail in the Python 3.5 environment only. However, when I tried to replicate this irregular selection of default releases for installation of NumPy via pip, I did not get the same results. Instead, only the latest version 1.15.0 was ever selected, as one would expect.

Unfortunately, the TravisCI build logs do not show which version of NumPy is installed, so I can't check my hypothesis right now. Since this appears to have nothing to do with axographio or the change made in the latest merger in this PR, I'm not sure why this wasn't a problem before but is now, nor why upsteam master is currently passing tests.

@jpgill86
Copy link
Contributor

I've characterized the problem I described above in a new issue: #556

This seems to be an issue either with Neo's master branch or with NumPy, but not with this PR nor with axographio.

@jpgill86
Copy link
Contributor

jpgill86 commented Sep 6, 2018

The problem with continuous integration described above and in #556 is now resolved and a fix has been merged into master. @will-hart, if you rebase these changes onto master, CI tests should pass, and hopefully AxographIO can be merged too.

@will-hart
Copy link
Contributor Author

@jpgill86 great, thanks. It might be a few days but I'll rebase as soon as I can.

@will-hart
Copy link
Contributor Author

OK, finally got around to rebasing today... and it looks like CircleCI is now failing with a bunch of warnings and deprecation messages.

@jpgill86
Copy link
Contributor

Hmm, looks like the commits you previously merged from will-hart#1, will-hart#2, and will-hart#3 were discarded. Can you merge them in again?

@will-hart
Copy link
Contributor Author

Oh yeah of course, I must have forgotten to pull the changes locally. My excuse is I'm a bit sleep deprived at the minute as we've just welcomed a new arrive into our family :). I'll sort it out now.

@jpgill86
Copy link
Contributor

Congratulations @will-hart, that sounds like happy news!

I'm beginning to suspect this PR is cursed! After I fixed a bug in the axographio package that was causing some tests to fail here, we uncovered a new set of broken tests that had nothing to do with this PR and which popped up because Travis CI changed their default build settings within a narrow window of time. After about a month, we fixed the bugs in Neo and finally had those tests working in Travis CI again. You rebased this PR onto the latest commit in master, which was created just one day earlier and which passed its tests, and amazingly the tests are failing here again!

I've started to characterize the new issue here: #568. Once again, the cause appears to be outside of this PR and, once again, it probably has to do with Circle CI changing their default build settings within that one-day window! Hopefully the new issue can be fixed soon, you can rebase again, tests will finally pass, and we can merge AxographIO into master, haha. 🤞

@will-hart
Copy link
Contributor Author

I'm not sure how much more of this emotional roller coaster I can take!!! 😄 OK, I'll wait for the go ahead to rebase again.

@muellerbjoern
Copy link
Contributor

@will-hart The tests are fixed in master now, if you rebase now they should hopefully pass.

@jpgill86
Copy link
Contributor

Tests are broken again. See #572. @will-hart, don't rebase yet.

@jpgill86
Copy link
Contributor

OK, tests are passing on master again. @will-hart, can you rebase once more? Quick, before something breaks again! 🤣

@jpgill86
Copy link
Contributor

@will-hart @JuliaSprenger @bjoern1001001 Tests are passing! Ready to merge!

@JuliaSprenger
Copy link
Member

Congratualtions, finallly everything works. Thanks for staying with this PR for so long!

@JuliaSprenger JuliaSprenger merged commit 603e049 into NeuralEnsemble:master Sep 18, 2018
@samuelgarcia
Copy link
Contributor

Vous êtes des champions!!

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.

8 participants