Skip to content

Added support for missing channels in SpikeGadgets #1593

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

Merged
merged 11 commits into from
Mar 26, 2025

Conversation

pauladkisson
Copy link
Contributor

Fixes #1592

@zm711
Copy link
Contributor

zm711 commented Nov 3, 2024

Do you have a small test file we could include. Some of these spike gadget edge cases are starting to bite us. I would love if we had some way to actually test against these edge cases so we don't break this in the future?

@pauladkisson
Copy link
Contributor Author

I have a test file, but it's not small (27GB). Do you know how to stub a spike gadgets .rec file?

@zm711
Copy link
Contributor

zm711 commented Nov 4, 2024

I think @samuelgarcia knows this format a bit better. So might be worth pinging him or @alejoe91 to see if they know how. Our limit is 10MB so 27GB is definitely over haha. We will see what se can do. :)

@pauladkisson
Copy link
Contributor Author

Actually, to test this fix we should just need the header, which should be relatively easy to stub. I'll see what I can do...

@pauladkisson
Copy link
Contributor Author

Ok, I was able to stub the file to just include the header (no data) and now it's only 72KB. Still awaiting permission from the original researchers to share with y'all, but once I get that, I will send over the stubbed file.

@zm711
Copy link
Contributor

zm711 commented Nov 5, 2024

Thanks Paul, that would be awesome. This was originally caused by trying to explicitly support Intan chips which was maybe the wrong move. Previously we just gave arbitrary names to the channels which worked for everything, but was unclear to the user. If we can name things correctly both for NP and Intan that would be the best, but I don't know this system at all. Really appreciate your help on this :)

@pauladkisson
Copy link
Contributor Author

Hey @zm711, Github doesn't support .rec as a file type, so I can't upload it here. What is the best way to share this file with you?

@zm711
Copy link
Contributor

zm711 commented Nov 5, 2024

The options are you can zip it and include on github or you can email it to me. Do you have a preference?

@pauladkisson
Copy link
Contributor Author

stubbed_files.zip

@pauladkisson
Copy link
Contributor Author

lmk if that works

@zm711
Copy link
Contributor

zm711 commented Nov 7, 2024

Super busy next couple days. Will check this weekend and let you know!

@zm711
Copy link
Contributor

zm711 commented Nov 10, 2024

@pauladkisson, the file looks fine. I don't actually have gin on my home computer. Will submit the file to the test repo tomorrow. Then we can make sure tests pass with the new file. Then final review.

@zm711 zm711 self-assigned this Nov 10, 2024
@zm711 zm711 added this to the 0.14.0 milestone Nov 10, 2024
Copy link
Contributor

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zm711 I know how to stub these files with data. Do you still have the ones from the previous issues? Let's have a call of this at some point.

@zm711
Copy link
Contributor

zm711 commented Nov 11, 2024

@zm711 I know how to stub these files with data. Do you still have the ones from the previous issues? Let's have a call of this at some point.

We don't have the old one, but if you want to submit Paul's file to gin for me to review that would be awesome. I'm trying to find time to do it, but super busy in lab right now.

@@ -16,6 +17,38 @@ class TestSpikeGadgetsRawIO(
"spikegadgets/SpikeGadgets_test_data_2xNpix1.0_20240318_173658.rec",
]

class TestSpikeGadgetsRawIOHeaderOnly(unittest.TestCase):
def setUp(self):
filename = "/Volumes/T7/CatalystNeuro/Jadhav/stubbed_files/SL18_D19_S01_F01_BOX_SLP_20230503_112642_stubbed.rec"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something tells me this line won't work for the CI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will need to be updated once the test file is up on the gin repo.

@alejoe91 alejoe91 modified the milestones: 0.14.0, 0.15.0 Jan 17, 2025
@pauladkisson
Copy link
Contributor Author

@zm711, any update on this?

@zm711
Copy link
Contributor

zm711 commented Mar 13, 2025

Sorry I got super busy and this fell off my radar. I'm pretty booked up next week. If I have time I'll try to add the file tomorrow. If you're in a hurry you could open a PR in Gin that I can review pretty quickly or you try to get Heberto to do it. But I'll put this back on my todo list!

@pauladkisson
Copy link
Contributor Author

@zm711, @h-mayorquin, any chance we can get this up this week?

@h-mayorquin
Copy link
Contributor

@zm711
Copy link
Contributor

zm711 commented Mar 24, 2025

Just to confirm this is only xml stuff? When I look at the .rec it seems to all be xml.

I'm asking because the other stubbed file in the repo goes beyond the xml into a little bit of binary file. So I want to make sure that we are sure this stub works before final merge @pauladkisson you've tested this stub locally too?--With my testing this works. So I'll merge into gin. remember to update your PR for the gin location.

@pauladkisson
Copy link
Contributor Author

pauladkisson commented Mar 24, 2025

Just to confirm this is only xml stuff? When I look at the .rec it seems to all be xml.

The stub only includes the header information (lines up to and including "</Configuration>"). This is because I wasn't confident I could include stubbed data without mangling it somehow.

I'm asking because the other stubbed file in the repo goes beyond the xml into a little bit of binary file. So I want to make sure that we are sure this stub works before final merge @pauladkisson you've tested this stub locally too?--With my testing this works. So I'll merge into gin. remember to update your PR for the gin location.

All the tests that I wrote only check the header, but I think that is sufficient to ensure the proper functionality here. If you think otherwise, I would need some help with stubbing the actual data.

@zm711
Copy link
Contributor

zm711 commented Mar 25, 2025

we will try testing and make sure this works :)

All the tests that I wrote only check the header, but I think that is sufficient to ensure the proper functionality here. If you think otherwise, I would need some help with stubbing the actual data.

No I think this good for me.

@zm711
Copy link
Contributor

zm711 commented Mar 25, 2025

@pauladkisson I didn't read your test carefully enough. What you actually need to do is this:

import unittest
from pathlib import Path

from neo.rawio.spikegadgetsrawio import SpikeGadgetsRawIO
from neo.test.rawiotest.common_rawio_test import BaseTestRawIO
from numpy.testing import assert_array_equal


class TestSpikeGadgetsRawIO(
    BaseTestRawIO,
    unittest.TestCase,
):
    rawioclass = SpikeGadgetsRawIO
    entities_to_download = ["spikegadgets"]
    entities_to_test = [
        "spikegadgets/20210225_em8_minirec2_ac.rec",
        "spikegadgets/W122_06_09_2019_1_fromSD.rec",
        "spikegadgets/SpikeGadgets_test_data_2xNpix1.0_20240318_173658.rec",
        "spikegadgets/SL18_D19_S01_F01_BOX_SLP_20230503_112642_stubbed.rec"
    ]

    def test_parse_header_missing_channels(self):

        file_path = Path(self.get_local_path("spikegadgets/SL18_D19_S01_F01_BOX_SLP_20230503_112642_stubbed.rec"))
        reader = SpikeGadgetsRawIO(filename = file_path)
        reader.parse_header()

        assert_array_equal(
            reader.header['signal_channels']['id'],
            [
                'ECU_Ain1', 'ECU_Ain2', 'ECU_Ain3', 'ECU_Ain4', 'ECU_Ain5', 'ECU_Ain6',
                'ECU_Ain7', 'ECU_Ain8', 'ECU_Aout1', 'ECU_Aout2', 'ECU_Aout3', 'ECU_Aout4', '0',
                '32', '96', '160', '192', '224', '1', '33', '65', '97', '161', '193', '225', '2', '34',
                '98', '162', '194', '226', '3', '35', '67', '99', '163', '195', '227', '4', '36',
                '100', '164', '196', '228', '5', '37', '69', '101', '165', '197', '229', '6', '38',
                '102', '166', '198', '230', '7', '39', '71', '103', '167', '199', '231', '8', '40',
                '72', '104', '136', '168', '200', '232', '9', '41', '73', '105', '137', '169', '201',
                '233', '10', '42', '74', '106', '138', '170', '202', '234', '11', '43', '75', '107',
                '139', '171', '203', '235', '12', '44', '76', '108', '140', '172', '204', '236', '13',
                '45', '77', '109', '141', '173', '205', '237', '14', '46', '78', '110', '142', '174',
                '206', '238', '15', '47', '79', '111', '143', '175', '207', '239', '80', '144', '176',
                '208', '240', '17', '49', '81', '145', '177', '209', '241', '82', '146', '178', '210',
                '242', '19', '51', '83', '147', '179', '211', '243', '84', '148', '180', '212', '244',
                '21', '53', '85', '149', '181', '213', '245', '86', '150', '182', '214', '246', '23',
                '55', '87', '151', '183', '215', '247', '24', '56', '88', '152', '184', '216', '248',
                '25', '57', '89', '121', '153', '185', '217', '249', '26', '58', '90', '154', '186',
                '218', '250', '27', '59', '91', '123', '155', '187', '219', '251', '28', '60', '92',
                '156', '188', '220', '252', '29', '61', '93', '125', '157', '189', '221', '253', '30',
                '62', '94', '158', '190', '222', '254', '31', '63', '95', '127', '159', '191', '223',
                '255',
            ]
        )

because you're using an institutional account I'm not allowed to push the fix.

@pauladkisson
Copy link
Contributor Author

@pauladkisson I didn't read your test carefully enough. What you actually need to do is this:

Done!

@zm711
Copy link
Contributor

zm711 commented Mar 25, 2025

Let's hope I didn't add any typos :). But yes we will see!

@zm711
Copy link
Contributor

zm711 commented Mar 25, 2025

So it looks to me like the stub might not quite have enough info. We might need to rewrite the test one more time. Let me reread what went wrong.

@zm711
Copy link
Contributor

zm711 commented Mar 25, 2025

@pauladkisson, everything looks good. I noticed an issue with our CI. I'm going to fix it and then once I have the CI fully working then I want to rerun tests (our 3.12 tests are skipping right now, but 3.9 are passing so we can merge this as soon as I get 3.12 working again :( )

Fixed in #1667. So once we get that merged we can update this PR and make sure 3.12 tests are passing.

@zm711
Copy link
Contributor

zm711 commented Mar 26, 2025

@pauladkisson could I have you merge main into this :). Then one more run of the test suite. Then we can merge.

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me. I'll merge.

@zm711 zm711 merged commit d265dc6 into NeuralEnsemble:master Mar 26, 2025
5 checks passed
@h-mayorquin h-mayorquin deleted the spikegadgets branch March 26, 2025 18:12
@zm711 zm711 modified the milestones: 0.15.0, 0.14.1 Mar 28, 2025
@zm711
Copy link
Contributor

zm711 commented Mar 28, 2025

@pauladkisson do you want to be added to our author list? I'm prepping a pr for a release/updating stuff. So I would need the name you want to appear and your affiliation.

@pauladkisson
Copy link
Contributor Author

Sure! My name is Paul Adkisson and I am a RSE at CatalystNeuro.

@zm711
Copy link
Contributor

zm711 commented Mar 29, 2025

Great. Will do :)

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.

SpikeGadgetsRawIO Gives incorrect channel ids with recording missing hardware channels
5 participants