Skip to content

Conversation

@nbara
Copy link
Contributor

@nbara nbara commented Apr 18, 2017

This is an attempt to allow reading GDF files in mne. The GDF format is described in detail here:
https://arxiv.org/abs/cs/0608052

This file format is currently only supported by the biosig toolbox (which is used by fieldtrip & Co). There is an active homebrew port of the biosig toolbox (https://github.com/schloegl/homebrew-biosig). This would probably be the cleanest way to ensure long-lasting GDF support in python (especially since the format is still likely to evolve), but I wasn't able to install it on my machine, and it's macOS-only...

Some of the code was taken from @alexandrebarachant's previous attempt (see #2136, and https://github.com/alexandrebarachant/mne-python/tree/gdf) and an apparently discontinued biosig python port, with the following improvements:

  • support for GDF 1.x and 2.x : this is a bit tricky since the format seems to have evolved a lot since its introduction, and AFAICT there is no backwards compatibility between the two. So I had no choice but write specific code for the two versions.
  • GDF support is embedded in the EDF/BDF module. This might sound nice, but it's not the cleanest piece of code either, so I would be glad if someone more experienced with eeg file formats could have a look (@teonbrooks maybe?)

A few other notes :

  • I only have tested 2.x support with files generated with biosemi2ft, which we use in the lab. We probably need to run more tests before merging but I couldn't find any publicly available dataset
  • @alexandrebarachant has provided a GDF 1.x test file, which I've attached to the present commit. Ideally we would also need more test files for version 2.x )
  • Event values for GDF 2.x files are detected as multiples of 256 on my test files, so I suspect there is still something fishy going on with the data types.
  • It's my first PR so any tips/comments welcome !

nbara added 3 commits April 17, 2017 21:18
only tested on a few GDF test files
doesn't break existing EDF tests
- Unified offset and calibration values for EDF/BDF/GDF modules
- few PEP8 fixes
- TODO: GDF 2.x triggers are detected as multiples of 256
- TODO: overflow and saturation detection
@larsoner
Copy link
Member

and an apparently discontinued biosig python port, with the following improvements:

This code is GPLv3, which is incompatible with our license (BSD). I think you'll need to contact the authors of that code to get permission to re-license the code as BSD :(

@larsoner
Copy link
Member

GDF support is embedded in the EDF/BDF module.

I think @alexandrebarachant suggested doing this, but if there is a lot of branching involved, it might be easier to have it be separate, and just have the two codebases rely on a shared set of private functions. It's hard to tell without working with the code directly, so it's really up to you which way is easier/cleaner.

@alexandrebarachant has provided a GDF 1.x test file, which I've attached to the present commit.

We should add this file to mne-testing-data instead to avoid repo bloat.

@nbara
Copy link
Contributor Author

nbara commented Apr 18, 2017

This code is GPLv3, which is incompatible with our license (BSD). I think you'll need to contact the authors of that code to get permission to re-license the code as BSD :(

There's not that much code overlap with biosig.py tbh since mne's data structure is quite different. At most, code overlap is the same as in #2136, and we can further reduce it. That said, some overlap is unavoidable imo.

But I guess we can ping @schloegl to see what he thinks ?

I think @alexandrebarachant suggested doing this, but if there is a lot of branching involved, it might be easier to have it be separate, and just have the two codebases rely on a shared set of private functions. It's hard to tell without working with the code directly, so it's really up to you which way is easier/cleaner.

It's not so bad currently in my opinion. Maybe the authors of edf.py will want to comment?
The branching mostly occurs in _get_edf_info(), and I could just as easily move the GDF code into a new _get_gdf_info(). Just let me know what you prefer :)


edf_info['events'] = events
edf_info['events'] = np.c_[POS, DUR, TYP]

Copy link
Member

Choose a reason for hiding this comment

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

Instead of burying these in very long conditionals, it would be nicer to use matched-API private functions like edf_info = _read_bdf_info(...), edf_info = _read_edf_info(...) or whatever the appropriate API would be.

Copy link
Contributor Author

@nbara nbara Apr 18, 2017

Choose a reason for hiding this comment

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

I can definitely do that but the code will start to diverge heavily from the existing edf.py file (not that I mind, it will just make it more difficult for others to track this PR)

I was thinking about replacing _get_edf_info(), with a generic _get_info() function, which would call either _read_edf_header() (for EDF and BDF files), or _read_gdf_header() (for GDF files), depending on the file extension. (I'm referring to 'headers' here as that's what they're called in about every EEG toolbox I've ever come across, so it would be understandable by most users)

I think it would be the cleanest option, as it would help reduce code repetition to a minimum.

Does that sound ok to you ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine. Looking at the diff, the code has already diverged a lot (lots of changed lines) so I don't think it will make that situation any worse.

@larsoner
Copy link
Member

There are a number of style issues travis will complain about. You can do make pep locally (in the mne-python directory) to run all style-related tests. You'll need pydocstyle and flake8 installed.

@nbara
Copy link
Contributor Author

nbara commented Apr 18, 2017

Ok thanks Eric! I've run autopep8 on all the files but will also do that.

Also, I have just realised that I was missing some of the EDF test files from mne-testing-data, and saw that I broke some functionality related to stim channels. Will try to fix that asap.

nbara added 2 commits April 18, 2017 16:39
An additional warning has been added in edf.py when Lowpass information
is not available in file header.

As a result test_stim_channel() would fail. The test should now expect
2 warnings.
- EDF and GDF file headers are now read in their own private subfunction
- some PEP8 and cosmetic changes
- GDF events are still multiples of 256
@nbara
Copy link
Contributor Author

nbara commented Apr 18, 2017

OK I've split the EDF/BDF and GDF header reads, what do you think ?

make Travis happy
@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #4205 into master will decrease coverage by 0.06%.
The diff coverage is 77.75%.

@@            Coverage Diff             @@
##           master    #4205      +/-   ##
==========================================
- Coverage   86.24%   86.18%   -0.07%     
==========================================
  Files         357      358       +1     
  Lines       64576    65007     +431     
  Branches     9832     9917      +85     
==========================================
+ Hits        55695    56026     +331     
- Misses       6180     6246      +66     
- Partials     2701     2735      +34

- python3 requires decoding the bytes objects to produce strings
- np.unique() fails for dtype=object in python3 (see
numpy/numpy#641)
- flake, pydocstyle
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.

Looks like good progress.

Don't forget to move the testing files to mne-testing-data repo instead of here.

# GDF data
elif subtype == 'GDF':
ch_data = np.fromfile(fid, dtype=gdftype, count=samp)
ch_data = np.float64(ch_data)
Copy link
Member

Choose a reason for hiding this comment

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

We usually just do .astype(np.float64) instead of np.float64(...)

else:
edf_info['data_size'] = 2 # 16-bit (2 byte) integers
raise NotImplementedError(
'Only GDF, EDF, and BDF files are supported.')
Copy link
Member

Choose a reason for hiding this comment

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

', got %s' % (ext,)

__gdftyp_np.append(None)
__gdftyp_np.append(None)
__gdftyp_np.append(np.float32)
__gdftyp_np.append(np.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Can you just make this list instead of doing a bunch of appends? It saves a lot of lines

Copy link
Member

Choose a reason for hiding this comment

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

... and you can do this, too:

__gdftyp_byte = [np.dtype(x).itemsize if x is not None else 0 for x in __gdftyp_np]

And to sanity check you can add something like:

assert sum(__gdftyp_byte) == ...

Just to sanity check (which is nice).

Also we don't prefix variables with __. You don't need them, gdftype_byte is fine already.

encoded as 4256 (Volt) + 19 (micro) = 4275. The code 512
stands for dimensionless.
""" # noqa
[fid.read(6) for ch in channels] # phys_dim, obsolete
Copy link
Member

Choose a reason for hiding this comment

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

fid.seek(6 * len(channels), 1)

stands for dimensionless.
""" # noqa
[fid.read(6) for ch in channels] # phys_dim, obsolete
units = [np.fromstring(fid.read(2), np.uint16)[0]
Copy link
Member

Choose a reason for hiding this comment

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

np.fromfile

units[i] = 1
else:
warn('Unknown physical dimension for channel %d. '
'Assuming dimensionless.' % i)
Copy link
Member

Choose a reason for hiding this comment

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

have you hit this case? Maybe we should just raise an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the GDF format supports a whole bunch of other dimensions (such as Ohm, %, degree, etc.), and apparently also supports undefined dimensions (code 0).

Should I import the whole list? It's going to add a lot of bloat and I suspect not many users will need it.

Copy link
Member

Choose a reason for hiding this comment

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

You could add them to constants.py, but if it's a lot of work can you add a link to a URL or just mention that we can eventually expand the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed your comments except for this. I don't have much time to do this this week, and it's unlikely that you will need more than these 3 constants (uV, Ohm, and dimensionless).

I've added a distinction between "unrecognized" (unit ==0, supprted by GDF) and "unsupported" dimensions. Should I add something in the warning for the unsupported case?

Copy link
Member

Choose a reason for hiding this comment

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

Okay we can wait to do a constants.py for now, then. I think saying nothing for "unrecognized" but warn for "unsupported" (with suggestion to contact developers to add support), and set the channel as MISC type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did this, but it occurred to me that the the GDF format (especially earlier versions AFAICT) doesn't enforce units. So setting the channel type to MISC would be quite an inconvenience to the user (I mean, if I know my datafile contains EEG data it makes little sense for the code to change it post-hoc to MISC, doesn't it?).

SO I think the warning surely should suffice. WDYT?


ch_names = [ch_names[idx] for idx in include]
physical_min = np.array(
[np.fromstring(fid.read(8), np.float64)[0] for ch in channels])
Copy link
Member

Choose a reason for hiding this comment

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

np.fromfile. And don't iterate, just read the full set of elements at once. Same with all other reads. (It will go faster and use fewer lines.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? Wouldn't that mean I'd have to manually set fid.seek (using absolute positioning) for each param if I used np.fromfile everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

np.fromfile(fid, ...) is the same as np.fromstring(fid.read(...), ...

@nbara
Copy link
Contributor Author

nbara commented Apr 20, 2017

@Eric89GXL I've had to add an ugly biosemi toggle, that allows reading trigger channels from biosemi GDF files. This is due to biosemi hardware specifications.

I couldn't think of a more elegant way to do this, as there is absolutely no information anywhere in the GDF file which indicates that it was recorded from a biosemi system. What do you think?

Also, any idea why travis fails ? I can't decrypt the log...

I'm going to add the test files to the testing dataset now. I can include one of my own GDF 2.x files as well to increase coverage, but the smallest I have is~ 150Mb, is that OK ?

- fromstring -> fromfile
@larsoner
Copy link
Member

I couldn't think of a more elegant way to do this, as there is absolutely no information anywhere in the GDF file which indicates that it was recorded from a biosemi system. What do you think?

@jaeilepp or I could take a look once this is otherwise ready to go with test files, etc. Please don't let us forget :)

The Travis failure is indeed bizarre, never seen it. I restarted Travis.

Let me know if you can't record a shorter GDF file. I think I know some folks with a Biosemi that I can ask about it.

@agramfort
Copy link
Member

can we put the test file in mne-testing-data?

@larsoner
Copy link
Member

can we put the test file in mne-testing-data?

Yes (as suggested above) but it should still be much smaller than 150 MB. The goal is as small as possible.

@nbara
Copy link
Contributor Author

nbara commented Apr 20, 2017

The PR in the testing data repo is ready to go. I'll record some new GDF2.2 testing data tomorrow and make the PR with the GDF1 and GDF2 test files right after.

@larsoner
Copy link
Member

Okay, data added. Change the testing version number in mne/datasets/utils.py, then update the hash (which I usually get by trying to mne.datasets.testing.data_path(force_update=True, verbose=True) and looking at the error message).

assert_equal(len(w), 1)
assert_true('Events may jitter' in str(w[0].message))
assert_equal(len(w), 2)
assert_true('Events may jitter' in str(w[1].message))
Copy link
Member

Choose a reason for hiding this comment

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

What is the other new event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added this in line 467 a few commits back :

if lowpass.size == 0:
    info['sfreq'] / 2.
    warn('Lowpass info could not be found. Setting it at half the '
         'sampling rate')

Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need that warning. If it's not encoded in the file it's okay to just set it to half the sampling rate.

Copy link
Member

Choose a reason for hiding this comment

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

A logger.info(...) is probably sufficient

@nbara
Copy link
Contributor Author

nbara commented Apr 24, 2017

I've added a simple check for stim_channel (which must belong to the list of ch_names if it's a string, and not be greater than the total number of channels if it's an index), as discussed in #4217.

This is ready for review @agramfort @Eric89GXL

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.

Otherwise LGTM

def __init__(self, input_fname, montage, eog=None, misc=None,
stim_channel=-1, annot=None, annotmap=None, exclude=(),
preload=False, verbose=None): # noqa: D102
preload=False, verbose=None, biosemi=False): # noqa: D102
Copy link
Member

Choose a reason for hiding this comment

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

do we need this or can we get it from the file extension?

If we need to keep it, please update the docstring above

warn("Stimulus Channel will not be annotated. Both 'annot' and "
"'annotmap' must be specified.")

self.biosemi = biosemi
Copy link
Member

Choose a reason for hiding this comment

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

make private as self._biosemi

Copy link
Member

Choose a reason for hiding this comment

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

... and do we need this? Can we instead use self._raw_extras[fi]['subtype']?

2**17 - 1)

if biosemi:
stim = stim >> 8 # not sure why but it works
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if they use the first 8 bits for something else, in which case this might risk some loss of information. Have you looked to see if there is documentation somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this:

Bit 16: High when new Epoch is started
Bit 17: Speed bit 0
Bit 18: Speed bit 1
Bit 19: Speed bit 2
Bit 20: High when CMS is within range
Bit 21: Speed bit 3
Bit 22: High when battery is low
Bit 23: (MSB) High if ActiveTwo MK2

Here:
https://www.biosemi.com/faq/trigger_signals.htm

I'd say we can safely skip this. Maybe Bit16 could be useful, but I've been using a Biosemi device for years and have no idea how to specify these "Epochs"

Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to leave it in the native format, and tell the users that they can do a bit shift in the docs. Then we have all information in as close to the native format as possible, people can use these bits if they need to, and we can get rid of the biosemi argument. Or do you think that would be too much of an inconvenience for users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think most users who are coming from matlab toolboxes are used to have this done for them (I know EEGLab, FT, and SPM do that under the hood). So it will definitely be a bit disorienting to some.

Personally I'm not opposed to this at all though, but is there an easy way to bit-shift in find_events() ?

Copy link
Member

Choose a reason for hiding this comment

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

events = mne.find_events(...)
events[:, 2] >>= 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, yes indeed ! I'll remove the toggle from the code tomorrow then.

Re: Notes, you mean io.rst? I can update that at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Add a Notes section to the docstring of read_raw_edf and RawEDF. But updating io.rst would also be good.

Copy link
Contributor Author

@nbara nbara Apr 26, 2017

Choose a reason for hiding this comment

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

Ok I'm having second thoughts now.

I think I'd rather put this in a Notes section about dealing with BioSemi data than modify users' data.

The original EDF/BDF code in edf.py already modifies users' data, as the trigger values come out correct for bdf files (i.e. bits 16-23 of the status channel are discarded)

ch_data = np.fromfile(fid, dtype=dtype, count=samp * dtype_byte)
ch_data = ch_data.reshape(-1, 3).astype(np.int32)
ch_data = ((ch_data[:, 0]) +
            (ch_data[:, 1] << 8) +
            (ch_data[:, 2] << 16))
# 24th bit determines the sign
ch_data[ch_data >= (1 << 23)] -= (1 << 24)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... that seems like more of a bug than a feature to me.

I'm still +1 on making documentation about it instead of adding an option -- it's more conservative from an API perspective because we can always easily add some option event_shift or so later if we realize it's really helpful, whereas removing the biosemi arg or making it more general somehow is not so easy. So how about for now we don't do the shift, and if users (yourself included) find it too cumbersome we add something later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!


# EDF data: 16bit data
else:
ch_data = np.fromfile(fid, dtype='<i2', count=samp)
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified by, instead of calling it gdftype, call it data_type or dtype, and assign it in both cases of the conditional above. Then this can just be ch_data = np.fromfile(fid, dtype=dtype, count=samp) in both cases (no need for elif subtype == 'GDF').

if np.any(~np.isfinite(cals)):
idx = np.where(~np.isfinite(cals))[0]
warn('Scaling factor is not defined in following channels:\n' +
str(idx).strip('[]'))
Copy link
Member

Choose a reason for hiding this comment

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

This is actually unsafe if one has [ in a channel name (rare I know but...) you can do something more robust and with a nicer output as ', '.join(ch_names[ii] for ii in idx).

edf_info['physical_min'] = physical_min
edf_info['record_length'] = record_length
edf_info['ref'] = ref
edf_info['units'] = units
Copy link
Member

Choose a reason for hiding this comment

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

This can be much more compact as edf_info.update(...)

Copy link
Member

Choose a reason for hiding this comment

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

... or just set them as you read them in

if stim_channel.lower().replace(' ', '') ==
ch.lower().replace(' ', '')]
if casematch:
err += ' Closest match is "{}".'.format(casematch[0])
Copy link
Member

Choose a reason for hiding this comment

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

indentation is wrong

and :ref:`Logging documentation <tut_logging>` for more).
biosemi : bool (default False)
Set to True to when reading Biosemi data as GDF files, otherwise the
stimulus channel will not be properly parsed.
Copy link
Member

Choose a reason for hiding this comment

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

so we can just get it from the file extension, then, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, because you can have GDF files recorded from a biosemi system (through biosemi2ft for instance)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, this is related to what you said above.

Is the only effect a bit shift on the stim channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, yes.


# Assert data are almost equal
print(data.shape)
print(data_biosig.shape)
Copy link
Member

Choose a reason for hiding this comment

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

remove


# Assert data are almost equal
print(data.shape)
print(data_biosig.shape)
Copy link
Member

Choose a reason for hiding this comment

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

remove

@nbara nbara force-pushed the gdf-support branch 3 times, most recently from 00c2a7e to a90f22e Compare April 27, 2017 07:32
@nbara
Copy link
Contributor Author

nbara commented Apr 27, 2017

Updated io.rst and Notes. WDYT @Eric89GXL ?

@larsoner
Copy link
Member

Codecov complains, can you have a look to see if you have many non-covered lines?

@nbara
Copy link
Contributor Author

nbara commented Apr 27, 2017

Mmh there's the event table bit for the GDF2 branch (I don't have test files for that), but nothing else that I can think of...

@larsoner
Copy link
Member

Okay, LGTM +1 for merge

@larsoner larsoner changed the title MRG: GDF support MRG+1: GDF support Apr 27, 2017

GDF files can be read in using :func:`mne.io.read_raw_edf`.

https://arxiv.org/abs/cs/0608052
Copy link
Member

Choose a reason for hiding this comment

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

does this render as a link? I would insert it in a sentence

cal = np.array([ch['cal'] for ch in self.info['chs']])
gains = np.atleast_2d(self._raw_extras[fi]['units'] *
(physical_range / cal))
cal = np.atleast_2d((physical_range) / (cal)) # physical / digital
Copy link
Member

Choose a reason for hiding this comment

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

(physical_range) / (cal)
->
physical_range / cal


info = _empty_info(sfreq)
info['meas_date'] = calendar.timegm(date.utctimetuple())
info['meas_date'] = edf_info['meas_date']
Copy link
Member

Choose a reason for hiding this comment

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

@jaeilepp are you ok with this?

Copy link
Member

Choose a reason for hiding this comment

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

seems like this is equivalent (updating edf_info to have the same value).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Looks like it's the same data.

ch.lower().replace(' ', '')]
if casematch:
err += ' Closest match is "{}".'.format(casematch[0])
raise ValueError(err)
Copy link
Member

Choose a reason for hiding this comment

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

this code is duplicated. See above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch yes, thanks

@agramfort
Copy link
Member

our edf users

@alexandrebarachant @Slasnista does it break any code for you?


info = _empty_info(sfreq)
info['meas_date'] = calendar.timegm(date.utctimetuple())
info['meas_date'] = edf_info['meas_date']
Copy link
Member

Choose a reason for hiding this comment

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

seems like this is equivalent (updating edf_info to have the same value).


if lowpass.size == 0:
pass
info['sfreq'] / 2.
Copy link
Member

Choose a reason for hiding this comment

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

this isn't assigned or done in place

Copy link
Member

Choose a reason for hiding this comment

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

are we lowpass filtering if it is not listed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realised that _empty_info(sfreq) defaults to lowpass = sfreq/2, so I won't make any assignment here.

edf_info['dtype_np'] = np.uint8
else:
edf_info['dtype_byte'] = 2 # 16-bit (2 byte) integers
edf_info['dtype_np'] = np.int16
Copy link
Member

Choose a reason for hiding this comment

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

is np.int16 big endian by default?

Copy link
Contributor Author

@nbara nbara Apr 28, 2017

Choose a reason for hiding this comment

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

I believe it's little endian by default. <i2 is what the previous code was doing so we should be good here.

>>> dt = np.dtype('<i2')
>>> print dt
int16
>>> dt = np.dtype('>i2')
>>> print dt
>i2
>>> dt = np.dtype(np.int16)
>>> print dt
int16

unitcodes = np.array(units[:])
include = list()
for i, unit in enumerate(units):
if unit == 4275: # microvolts
Copy link
Member

Choose a reason for hiding this comment

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

agreed, it would be nice to have constants there for quick ref

@agramfort
Copy link
Member

LGTM


http://www.edfplus.info/specs/edf.html
EDF (European Data Format; seehttp://www.edfplus.info/specs/edf.html) and EDF+
(http://www.edfplus.info/specs/edfplus.html) are 16-bit formats
Copy link
Member

Choose a reason for hiding this comment

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

@larsoner
Copy link
Member

@larsoner larsoner merged commit d130b74 into mne-tools:master Apr 28, 2017
@nbara nbara deleted the gdf-support branch September 14, 2017 21:47
@nbara nbara mentioned this pull request Sep 18, 2017
4 tasks
@massich massich mentioned this pull request Nov 27, 2018
4 tasks
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.

6 participants