Skip to content

Conversation

@massich
Copy link
Contributor

@massich massich commented Feb 13, 2019

@hubertjb realized that in #5718 we used an incomplete version of the sleep dataset.
(It was not clear that this version was available so we also open MIT-LCP/physionet#97)

This PR updates the fetchers so that we use the full version of the data.

@massich massich changed the title [WIP] Update Temazepam dataset to the complete physiobank dataset [WIP] Update sleep physionet to the complete dataset Feb 13, 2019

data_path = _data_path # expose _data_path(..) as data_path(..)

BASE_URL='https://physionet.org/physiobank/database/sleep-edfx/sleep-telemetry/'
Copy link
Member

Choose a reason for hiding this comment

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

pep8 ...

@agramfort
Copy link
Member

@hubertjb I'll let you try first to confirm that it works for you.

thx @massich for taking a stab at this

@larsoner
Copy link
Member

I am getting failures on master

https://travis-ci.org/mne-tools/mne-python/jobs/492796093

Is it related?

@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #5932 into master will decrease coverage by 37.35%.
The diff coverage is 89.47%.

@@             Coverage Diff             @@
##           master    #5932       +/-   ##
===========================================
- Coverage   88.72%   51.37%   -37.36%     
===========================================
  Files         401      401               
  Lines       72354    72351        -3     
  Branches    12154    12154               
===========================================
- Hits        64197    37169    -27028     
- Misses       5219    33069    +27850     
+ Partials     2938     2113      -825

@massich
Copy link
Contributor Author

massich commented Feb 13, 2019

Is it related?

yes completely. They turned the data down. (see MIT-LCP/physionet#97 (comment))

regarding the fleck8 I was aware. We start migrating together with @hubertjb and we only changed BASE_URL for temazepam. We decided that we would continue tomorrow but I guess that now this is a hotfix due to errors in master.

I'm on it.

@larsoner
Copy link
Member

Great, thanks. Will merge once Travis is happy again. If there is an example that uses this dataset, please (make sure it still works and) make some small change to it so that CircleCI will run it.

@massich
Copy link
Contributor Author

massich commented Feb 13, 2019

There's a travis test that mimics what circle will find. Ah, ok now I see what you meant. modify the example to trigger the makefile.

@larsoner
Copy link
Member

The mimic will usually not be as good/complete as having CircleCI actually download the data and run the example so we can examine the output. So please at least try running the example locally so that we don't break CircleCI with this PR

@massich
Copy link
Contributor Author

massich commented Feb 13, 2019

If green this should go in as it is. There's a subsequent PR making use and testing the new data.

@massich massich changed the title [WIP] Update sleep physionet to the complete dataset [HOTFIX] Update sleep physionet to the complete dataset Feb 13, 2019
@massich
Copy link
Contributor Author

massich commented Feb 13, 2019

The doc builds with no problem on my machine

@larsoner
Copy link
Member

@massich can you fix the new flake errors? If not I can do it

@larsoner
Copy link
Member

mne/datasets/sleep_physionet/age.py:17:1: E302 expected 2 blank lines, found 1
mne/datasets/sleep_physionet/_utils.py:149:5: F841 local variable 'subjects_url' is assigned to but never used
mne/datasets/sleep_physionet/tests/test_physionet.py:20:1: F401 'mne.datasets.sleep_physionet._utils.BASE_URL' imported but unused

@larsoner larsoner merged commit 90b91a3 into mne-tools:master Feb 13, 2019
@larsoner
Copy link
Member

Thanks @massich

@massich
Copy link
Contributor Author

massich commented Feb 13, 2019 via email

@massich massich deleted the sleep_dataset branch May 27, 2019 12:13
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