Repository navigation
Added Event Segmentation class, along with example and tests - #72
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
Add to whitelist. |
|
|
||
| class EventSegment: | ||
| """Class for learning and representing an event segmentation of | ||
| continuous fMRI data |
There was a problem hiding this comment.
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.
|
It seems like you are close to implementing the scikit-learn Estimator and Predictor interfaces: You should rename your |
|
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, :]))) |
There was a problem hiding this comment.
Could you add a simple plot? We like colorful stuff. It's really a question, it's not mandatory.
|
@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
|
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. |
| restructuredtext-lint | ||
| sphinx | ||
| sphinx_rtd_theme | ||
| nose |
There was a problem hiding this comment.
Please do not add an extra test framework. Please use pytest.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
|
@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. |
|
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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Very good point - I'll only modify copies.
|
@lcnature Are you happy with this PR? |
| @@ -0,0 +1,14 @@ | |||
| # Copyright 2016 Intel Corporation | |||
There was a problem hiding this comment.
This should be Princeton or your name.
|
@mihaic @cbaldassano |
|
Thank you very much for your contribution, @cbaldassano. And thanks for the review, @lcnature. |
…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.
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.