Skip to content

Conversation

dominikwelke
Copy link

@dominikwelke dominikwelke commented Sep 3, 2025

fixed some bugs in the reader:

  • handle . characters in file paths
  • handle floating point numbers in header fields
  • fix: handle fFrequency=0 case before adding fFrequency to datainfo dict
  • fix: wrong lines selected in annotation slice

@dominikwelke
Copy link
Author

hi @drammock
can we bump the version again?
will need this to progress mne-tools/mne-python#13176

@drammock
Copy link
Member

drammock commented Sep 3, 2025

I can review tomorrow (Thursday). No objection to bumping version after that

@dominikwelke
Copy link
Author

thanks! no pressure :)

curryreader.py Outdated
Comment on lines 170 to 171
elif text.replace('.','',1).isnumeric() : # BUG (float not recognized) FIXED
a[i] = float(text) # assign if it was a number
Copy link
Member

Choose a reason for hiding this comment

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

Since there's nothing else after this elif block, I think it's a bit cleaner to do the try-except method (even if it's a tad slower). This isn't a performance bottleneck (the loop only runs 16 times). Also the .replace(...).isnumeric() method won't handle negative numbers (probably that won't matter given the names of the fields above, they all appear to be strictly positive quantities, but better safe than sorry?)

Suggested change
elif text.replace('.','',1).isnumeric() : # BUG (float not recognized) FIXED
a[i] = float(text) # assign if it was a number
try:
a[i] = float(text)
except ValueError:
pass

curryreader.py Outdated

if fFrequency == 0 or fSampleTime != 0:
if fFrequency == 0. or fSampleTime != 0.: # BUG (order) FIXED
Copy link
Member

Choose a reason for hiding this comment

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

Just so I'm clear, the bug here was adjusting fFrequency after it was entered into the datainfo dict, right? If so, let's avoid changes to this line, as there wasn't anything broken about it before, and we should try to minimize the diff with upstream so it's easier to pull in any upstream changes down the road.

Suggested change
if fFrequency == 0. or fSampleTime != 0.: # BUG (order) FIXED
if fFrequency == 0 or fSampleTime != 0:

curryreader.py Outdated
@@ -426,7 +426,7 @@ def read(inputfilename='', plotdata = 1, verbosity = 2):
tixstop = contents.find('REMARK_LIST END_LIST')

if tixstart != -1 and tixstop != 1 :
annotations = contents[tixstart:tixstop - 1].splitlines()
annotations = contents[tixstart+1:tixstop].splitlines() # BUG (does not read correct lines) FIXED
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick] I'm unsure if the comment will still be helpful in 6 months. On the assumption that we only make changes to the reader if there was a demonstrable bug (i.e., no new features), the diff will tell us where the bugfixes were, and "does not read correct lines" seems (to me at least) inferrable from the change of indices in the slice. Feel free to disregard though, if you disagree.

Suggested change
annotations = contents[tixstart+1:tixstop].splitlines() # BUG (does not read correct lines) FIXED
annotations = contents[tixstart+1:tixstop].splitlines()

@dominikwelke
Copy link
Author

dominikwelke commented Sep 5, 2025

thanks for the suggestions @drammock - all make sense, all accepted

@drammock
Copy link
Member

drammock commented Sep 5, 2025

I've updated the PR's description to describe the 4 bugs addressed here. That should hopefully make it easier in future if we need to revisit / rebase / whatever. Thanks @dominikwelke

@drammock drammock merged commit 3b164b5 into mne-tools:main Sep 5, 2025
2 checks passed
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