Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Enable save/read, simplify "combine" kwarg and allow symmetric RAM optimization #20

Merged
merged 15 commits into from
Jul 13, 2021
Merged

[WIP] Enable save/read, simplify "combine" kwarg and allow symmetric RAM optimization #20

merged 15 commits into from
Jul 13, 2021

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Jul 8, 2021 •
edited by larsoner
Loading

PR Description

Closes: #16
Closes: #18
Closes: #10

  • Reading/writing occurs as netCDF files
  • Refactoring combine in the envelope_correlation function into EpochXXX
  • Allow symmetric RAM optimization

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@adam2392
Copy link
Member Author

adam2392 commented Jul 8, 2021 •
edited
Loading

Looks like I'm getting hit by an error that I can't reproduce locally, but only on Ubuntu and some Mac distros:

mne_connectivity/tests/test_connectivity.py:177: in test_io
270
    conn.save(fname)
271
mne_connectivity/base.py:481: in save
272
    self.xarray.to_netcdf(fname, mode='w')
273
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/xarray/core/dataarray.py:2778: in to_netcdf
274
    return dataset.to_netcdf(*args, **kwargs)
275
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/xarray/core/dataset.py:1799: in to_netcdf
276
    return to_netcdf(
277
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/xarray/backends/api.py:1092: in to_netcdf
278
    store.close()
279
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/xarray/backends/scipy_.py:237: in close
280
    self._manager.close()
281
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/xarray/backends/file_manager.py:222: in close
282
    file.close()
283
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:294: in close
284
    self.flush()
285
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:404: in flush
286
    self._write()
287
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:416: in _write
288
    self._write_var_array()
289
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:464: in _write_var_array
290
    self._write_var_metadata(name)
291
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:485: in _write_var_metadata
292
    self._write_att_array(var._attributes)
293
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:445: in _write_att_array
294
    self._write_att_values(values)
295
../../../hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/scipy/io/netcdf.py:557: in _write_att_values
296
    nc_type = REVERSE[values.dtype.char, values.dtype.itemsize]
297
E   KeyError: ('U', 4)

Will need to do some googling...

@adam2392 adam2392 requested review from britta-wstnr and larsoner July 9, 2021 20:49
britta-wstnr
britta-wstnr reviewed Jul 10, 2021
View reviewed changes
Copy link
Member

@britta-wstnr britta-wstnr left a comment

Choose a reason for hiding this comment

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

Regarding the error: looks like the attributes are making trouble?

mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/envelope.py Outdated Show resolved Hide resolved
mne_connectivity/envelope.py
conn = EpochTemporalConnectivity(
data=corr,
names=names,
times=times,
method='envelope correlation',
indices=indices,
indices='symmetric',
Copy link
Member

Choose a reason for hiding this comment

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

Why do you set it explicitly here when it is only set to 'symmetric' if indices is None in line 166? If output is always symmetric, are line 165-166 even needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of enabling indices to allow for a subset to be computed, since any connectivity function that is a bivariate function f(signal_1, signal_2) can allow for indices to define a subset.

Then this line would be needed since the default would be "all-to-all symmetric". WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so you are anticipating future functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

Well... now that you bring it up, I probably should've just implemented it first. But yeah figured this is the way the code will look like once I add lines to account for indices = ([...], [...]).

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to try adding now that I've addressed the other comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, upon re-reading the code, idk if that's worth doing.

Envelope correlation is already very easy to get, so idk if it's worth adding the indices unless it comes up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to table this and move onwards w/ other connectivity functions and examples. I figure we can add these niceties if ppl really want them later on.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

larsoner
larsoner reviewed Jul 12, 2021
View reviewed changes
examples/mne_inverse_envelope_correlation.py Outdated Show resolved Hide resolved
doc/api.rst Outdated Show resolved Hide resolved
mne_connectivity/base.py Outdated Show resolved Hide resolved
mne_connectivity/base.py Show resolved Hide resolved
@adam2392
Copy link
Member Author

Regarding the error: looks like the attributes are making trouble?

Ah looks like "netcdf4" library needs to be added into the requirements files :p

@adam2392 adam2392 merged commit 5fc3607 into main Jul 13, 2021
@adam2392 adam2392 deleted the improve branch September 8, 2021 14:20
tsbinns pushed a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
@adam2392
[MRG] Enable save/read, simplify "combine" kwarg and allow symmetric …
71b8a65 
…RAM optimization (mne-tools#20)

* Adding reading/writing capability with netCDF4 files
* simplified "combine" argument in `envelope_correlation`
* allow storage of symmetric connectivity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@britta-wstnr britta-wstnr Awaiting requested review from britta-wstnr

@larsoner larsoner Awaiting requested review from larsoner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@adam2392 @larsoner @britta-wstnr