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.

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

Update to eventseg #266

Merged
merged 4 commits into from Sep 20, 2017
Merged

Update to eventseg #266

merged 4 commits into from Sep 20, 2017

Conversation

cbaldassano
Copy link
Collaborator

Minor update to eventseg to support additional use case, added ipython notebook eventseg example, also updated download link in isfc example

Copy link
Member

@mihaic mihaic left a comment

Choose a reason for hiding this comment

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

Great looking tutorial! So far, we have been committing example code without outputs, for ease of tracking changes. Clearly it is helpful to have the outputs visible on the web. I will merge your PR (after you address the minor comment) and try to solve the problem in issue #268.

@@ -345,7 +364,11 @@ def find_events(self, testing_data, var=None, scramble=False):
"""

if var is None:
var = self.event_var_
if self.event_var_ is None:
Copy link
Member

Choose a reason for hiding this comment

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

The test should check the existence of the attribute, not whether it is None. The documentation is not perfectly clear, but the code is:

the existence of parameters with trailing _ is used to check if the estimator has been fitted.
http://scikit-learn.org/stable/developers/contributing.html#parameters-and-init

    if not all_or_any([hasattr(estimator, attr) for attr in attributes]):
        raise NotFittedError(msg % {'name': type(estimator).__name__})

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/validation.py#L793

@mihaic
Copy link
Member

mihaic commented Sep 19, 2017

You should also update the paper reference to point to Neuron.

@mihaic mihaic merged commit 785599a into brainiak:master Sep 20, 2017
@cbaldassano cbaldassano deleted the eventseg_Sept17 branch September 20, 2017 18:18
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.

None yet

2 participants