Skip to content

Conversation

@withmywoessner
Copy link
Contributor

Reference issue

#6537
#11802

What does this implement/fix?

Change how number of samples is read and verifies that they are correct.

Additional information

I need to upload new testing data that demonstrates this bug.

@withmywoessner withmywoessner changed the title Change how sample num is read Fix bug with how Neuroscan .cnt n_samples is read Jan 29, 2024
@withmywoessner withmywoessner changed the title Fix bug with how Neuroscan .cnt n_samples is read [WIP][BUG] Fix bug related to how Neuroscan .cnt n_samples is read Jan 29, 2024
@withmywoessner
Copy link
Contributor Author

withmywoessner commented Jan 29, 2024

@larsoner Can I update the testing data to include a file that was not read properly before this change or even replace the one I already added.

The documentation said that the header I am using was not reliable, but I think that may be because they read the n_samples as normal int when it should be an unsigned int. The eeglab load_cnt function reads the n_samples properly and they also read it as an unsigned int.

I am not sure why this broke a bunch of tests. The cnt.py tests ran fine and I plotted all the testing data and it all looks fine.

@larsoner
Copy link
Member

Can I update the testing data to include a file that was not read properly before this change or even replace the one I already added.

Yes feel free! The file is small, right? The smaller the testing dataset the better, so might as well replace the old one assuming we can still test all the same stuff.

Comment on lines +339 to +346
annotations = _read_annotations_cnt(input_fname, data_format="int16")
if annotations.onset[-1] * sfreq > n_samples:
n_bytes = 4
n_samples = n_samples_header
warn(
"Annotations are outside data range. "
"Changing data format to 'int32'."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larsoner I added a check to see if there were any annotations outside the actual amount of data. If there are, I switch the data_format. I could not figure out a way to detect the correct number of samples with certainty. It only triggers when using the 'auto' data_format which already doesn't work with the current testing data I uploaded. Is this fine with you?

Comment on lines 90 to 99
@testing.requires_testing_data
def verify_reading_bytes():
raw_16 = read_raw_cnt(fname, preload=True)
raw_32 = read_raw_cnt(fname_bad_spans, preload=True)

# Verify that the number of bytes read is correct
assert len(raw_16) == 3070
assert len(raw_32) == 90000


Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I was unable to run my test locally using pytest

@drammock
Copy link
Member

drammock commented Jan 30, 2024 via email

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

And don't forget to change the hash and version number of the testing dataset in mne/datasets/config.py

@withmywoessner
Copy link
Contributor Author

Done! @larsoner New hash and version is in the config file:
image
Everything should be good to go! Thanks!

@larsoner
Copy link
Member

CIs are unhappy, can you look?

@withmywoessner
Copy link
Contributor Author

withmywoessner commented Jan 31, 2024

CIs are unhappy, can you look?

I forgot a docustring. It should hopefully be good @larsoner. I think this is causing it to fail now:

_no_parse = pytest.warns(RuntimeWarning, match="Could not parse")

@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

Looking at the logs the error is:

mne/io/cnt/tests/test_cnt.py:57: in test_auto_data
    with pytest.warns(RuntimeWarning, match="Omitted 6 annot"):
E   RuntimeWarning: Could not define the number of bytes automatically. Defaulting to 2.

So I think you need to change the test now to catch a different warning string (or maybe multiple warning strings with multiple pytest.warns)

@withmywoessner
Copy link
Contributor Author

withmywoessner commented Feb 2, 2024

Looking at the logs the error is:

mne/io/cnt/tests/test_cnt.py:57: in test_auto_data
    with pytest.warns(RuntimeWarning, match="Omitted 6 annot"):
E   RuntimeWarning: Could not define the number of bytes automatically. Defaulting to 2.

So I think you need to change the test now to catch a different warning string (or maybe multiple warning strings with multiple pytest.warns)

Oh, I thought it was the _no_parse test causing it to fail. I saw this when looking at the log:
image
I removed it and they pass now. Maybe I missed something? @larsoner

@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

Are you sure tests are passing and not being skipped at your end? In your mne-testing-data PR you had an outdated version in your version.txt locally, maybe that's causing a problem. Or maybe you're not on pytest>=8 where they changed the warning catching behavior. In any case, I could replicate the error locally and pushed a commit to fix it.

@larsoner larsoner marked this pull request as ready for review February 2, 2024 19:30
@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

@withmywoessner ready to go from your end? I clicked the "ready for review" button in case but let me know either way (and no need to put [wip] in your title, it's clear from draft status!)

@withmywoessner
Copy link
Contributor Author

Or maybe you're not on pytest>=8 where they changed the warning catching behavior. In any case, I could replicate the error locally and pushed a commit to fix it.

This fixed it. Thank you!

@withmywoessner
Copy link
Contributor Author

@withmywoessner ready to go from your end? I clicked the "ready for review" button in case but let me know either way (and no need to put [wip] in your title, it's clear from draft status!)

I am @larsoner. Thanks!

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Just one minor tweak that I'll commit, thanks @withmywoessner !

@larsoner larsoner changed the title [WIP][BUG] Fix bug related to how Neuroscan .cnt n_samples is read BUG: Fix bug related to how Neuroscan .cnt n_samples is read Feb 2, 2024
@larsoner larsoner enabled auto-merge (squash) February 2, 2024 21:26
@larsoner larsoner merged commit 4cea4a9 into mne-tools:main Feb 5, 2024
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
…ls#12393)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

3 participants