Skip to content

Brennan fixes#36

Open
SeanNobel wants to merge 35 commits intomainfrom
brennan-fixes
Open

Brennan fixes#36
SeanNobel wants to merge 35 commits intomainfrom
brennan-fixes

Conversation

@SeanNobel
Copy link
Owner

No description provided.

import numpy as np

# assert torch.cuda.is_available(), "Training without GPU is not supported."
DEVICE = torch.device("cuda" if torch.cuda.is_available() else "cpu")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This really shouldn't be hardcoded, unless for some reason it's not possible to run the code without a GPU (being too slow doesn't count). If you need to disable the GPU then that should be a flag.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Understand. I've put back device and bar_format to how it was in tidy.

# NOTE: I'm using GPU on my machine so heavily on other project that I can't use
# device = "cpu"

BRAIN_RESAMPLE_RATE = 120
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these added to constants.py instead of being in the config as before?

Copy link
Owner Author

@SeanNobel SeanNobel Apr 19, 2023

Choose a reason for hiding this comment

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

I thought those that are not hyperparameters and shouldn't be changed could be in constants.py?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I think in that case it should be OK (Python doesn't have any recommended way of dealing with this and constants.py is one solution) - but if the code can be run on CPU then definitely DEVICE should not be there and code should be device-agnostic.

@SeanNobel
Copy link
Owner Author

@Kaixhin After doing some visualizations with Brennan, I think the dataset class is not doing anything really wrong. (except that I cannot find the relative relationship of word and EEG onsets)
https://github.com/SeanNobel/speech-decoding/blob/brennan-fixes/notebooks/test_brennan2018.ipynb

But,

  • Looking at this subject 1, channel 48 is pretty noisy and probably should be removed in usual Neuroscience research. There should be channels like this in other subjects.
  • Audio embeddings look so similar from human eyes. (probably caused by the green feature around 700)

Will be posting the loss curve for current version later.

@Kaixhin
Copy link
Collaborator

Kaixhin commented Apr 19, 2023

@Kaixhin After doing some visualizations with Brennan, I think the dataset class is not doing anything really wrong. (except that I cannot find the relative relationship of word and EEG onsets) https://github.com/SeanNobel/speech-decoding/blob/brennan-fixes/notebooks/test_brennan2018.ipynb

Thanks for checking, good to note, but also I think your assumptions seem reasonable.

  • Looking at this subject 1, channel 48 is pretty noisy and probably should be removed in usual Neuroscience research. There should be channels like this in other subjects.

To a non-expert eye (me), I can't tell any channels are good or bad 😕 Unless the paper said otherwise, I think they kept all channels, and expect the DL model to just ignore noisy ones? Certainly a subject-specific layer could do that.

  • Audio embeddings look so similar from human eyes. (probably caused by the green feature around 700)

Looking very carefully, the horizontal bands seem similar (freq constant over time, makes sense), but I can see differences in the vertical bands, so I think it's OK.

@SeanNobel
Copy link
Owner Author

The scale of channel 48 of subject 1 after segmenting is thousands, while others are tens.
Due to that channel 48 is clipping so much after clamping. (we could avoid this by scaling each channel independently)

@Kaixhin
Copy link
Collaborator

Kaixhin commented Apr 19, 2023

(we could avoid this by scaling each channel independently)

Ah, this seems like it would be normal practice from an ML perspective. If the paper doesn't explicitly say otherwise, probably we should try this?

@SeanNobel
Copy link
Owner Author

CodeRabbit test

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