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

Added Event Segmentation class, along with example and tests #72

Merged
merged 1 commit into from Aug 29, 2016
Merged

Added Event Segmentation class, along with example and tests #72

merged 1 commit into from Aug 29, 2016

Conversation

@cbaldassano
Copy link
Collaborator

@cbaldassano cbaldassano commented Jul 19, 2016

Added event segmentation based on a hidden markov model. The paper describing this method is currently being written and will be published as a preprint within the next couple months.

@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Jul 19, 2016

Can one of the admins verify this patch?

1 similar comment
@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Jul 19, 2016

Can one of the admins verify this patch?

@mihaic
Copy link
Contributor

@mihaic mihaic commented Jul 19, 2016

Add to whitelist.


class EventSegment:
"""Class for learning and representing an event segmentation of
continuous fMRI data
Copy link
Contributor

@mihaic mihaic Jul 20, 2016

Please write a one-line summary (+ additional paragraphs for details, if needed):
https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

We have issue #65 open for adding this check to pr-check.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Jul 20, 2016

It seems like you are close to implementing the scikit-learn Estimator and Predictor interfaces:
http://scikit-learn.org/stable/developers/contributing.html#apis-of-scikit-learn-objects

You should rename your learn_events method to fit and your find_events method to predict. Furthermore, you should add a trailing underscore to all data-dependent attributes set after fitting the model and not set them (even to None) outside fit:
http://scikit-learn.org/stable/developers/contributing.html#estimated-attributes

@mihaic
Copy link
Contributor

@mihaic mihaic commented Jul 20, 2016

Please remove the underscore from the subpackage name, as we did with PR #73.

print("Boundary match: {:.2} (null: {:.2})".format(
np.mean(bound_match[0, :]), np.mean(bound_match[1, :])))
print("Log-likelihood: {:.3} (null: {:.3})".format(
np.mean(LL[0, :]), np.mean(LL[1, :])))
Copy link
Contributor

@mihaic mihaic Jul 20, 2016

Could you add a simple plot? We like colorful stuff. It's really a question, it's not mandatory.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Jul 25, 2016

@cbaldassano Could you please name someone who is knowledgeable about your project, either from PNI or Intel?

Making changes based on pull request comments, including implementing the sklearn Estimator interface

Improved simulated data example,  added sklearn validation test, added correctness test. Final commit before rebase.

Adding nose requirement and fixing docstrings

Removed sklearn tests

Fixed functions that were modifying their arguments, and added clarifying comments

Fixed docstrings
@cbaldassano
Copy link
Collaborator Author

@cbaldassano cbaldassano commented Aug 5, 2016

I've made all the requested changes! @lcnature agreed last week to help review this code - Mingbo, I will send you the preprint describing the method to help you understand what it is supposed to be doing.

@@ -6,3 +6,5 @@ pytest-cython
restructuredtext-lint
sphinx
sphinx_rtd_theme
nose
Copy link
Contributor

@mihaic mihaic Aug 5, 2016

Please do not add an extra test framework. Please use pytest.

Copy link
Collaborator Author

@cbaldassano cbaldassano Aug 5, 2016

I added this because scikit-learn has its own set of test functions I was called that required nose. However those test functions are causing more trouble then they are worth (they are failing in python 3.4, which is causing the travis failure) so I'm just going to remove them and the nose dependency.

Copy link
Contributor

@mihaic mihaic Aug 5, 2016

Oh, I see. We can include nose in this case, if it actually works.

The lesson I learned is that comments for a demo during a meeting are not high quality. :)

Copy link
Collaborator Author

@cbaldassano cbaldassano Aug 9, 2016

That's fine - if the sklearn tests weren't causing so many issues I would argue for including nose, but since I don't want to spend a ton of time debugging Python 3.4 pickling errors I took them out for now. I can say that the sklearn interface tests were passing at least in 3.5, so the current code meets all the primary BaseEstimator requirements.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Aug 9, 2016

@cbaldassano Have you sent Mingbo the preprint? Actually, are you considering publishing it on Arxiv or a similar place? If you are you could cite it in a docstring.

@cbaldassano
Copy link
Collaborator Author

@cbaldassano cbaldassano commented Aug 9, 2016

Yes, Mingbo has the preprint and said he will review the code soon. We'll be posting it on bioRxiv in a month or so, after we make some revisions, so maybe I'll submit a tiny pull request at that time to add the reference in the docstring.

X = [X]

n_train = len(X)
for i in range(n_train):
Copy link
Contributor

@lcnature lcnature Aug 25, 2016

I wonder if this can be avoided, e.g., by making copy of X and transpose the copy?
Because X is passed as reference, this line and line 118 which modify X will modify the data X outside the scope of this function, which might be unexpected by the user. For example, if a user wants to fit the data for multiple times, each time trying a different choice of K, (s)he might find the data has been modified after each time of fitting.

Copy link
Collaborator Author

@cbaldassano cbaldassano Aug 26, 2016

Very good point - I'll only modify copies.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Aug 26, 2016

@lcnature Are you happy with this PR?

@@ -0,0 +1,14 @@
# Copyright 2016 Intel Corporation
Copy link
Contributor

@mihaic mihaic Aug 26, 2016

This should be Princeton or your name.

@lcnature
Copy link
Contributor

@lcnature lcnature commented Aug 28, 2016

@mihaic @cbaldassano
Sorry for the delay. I am happy with all the changes 👍

@mihaic mihaic merged commit 68f04d5 into brainiak:master Aug 29, 2016
3 checks passed
@mihaic
Copy link
Contributor

@mihaic mihaic commented Aug 29, 2016

Thank you very much for your contribution, @cbaldassano. And thanks for the review, @lcnature.

danielsuo pushed a commit that referenced this issue Nov 16, 2017
…ts (#72)

* Give more informative error message when we do not know how to serialize a class.

* Check that passing arguments to remote functions and getting them does not change their values.

* fix serialization bug

* fix tests for common module

* Formatting.

* Bug fix in init_pickle_module signature.

* Use pickle with HIGHEST_PROTOCOL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants