8000 Add a score function for SRM by Kajiyu · Pull Request #478 · brainiak/brainiak · GitHub
[go: up one dir, main page]

Skip to content

Add a score function for SRM - #478

Open
Kajiyu wants to merge 4 commits into
brainiak:masterfrom
CaiLab-neuro:add_score_for_srm
Open

Kajiyu wants to merge 4 commits into
brainiak:masterfrom
CaiLab-neuro:add_score_for_srm

Conversation

@Kajiyu
@Kajiyu Kajiyu commented Aug 13, 2020
Copy link
Copy Markdown

This adds a function on SRM which calculates the log-likelihood of test-subjects' data for model selection.
We followed a style of scikit-learn: https://scikit-learn.org/stable/modules/generated/sklearn.mixture.GaussianMixture.html#sklearn.mixture.GaussianMixture.score

Co-authored-by: @lcnature .

@lcnature
lcnature commented Aug 14, 2020 •
Copy link
Copy Markdown
Contributor

This PR partially deals with #69 by adding cross-validated logp for SRM.

@mihaic
mihaic commented Aug 24, 2020
Copy link
Copy Markdown
Member

@qihongl, could you please review?

@qihongl
qihongl commented Feb 23, 2021
Copy link
Copy Markdown
Member

Oops sorry about the delayed reply! @lcnature @Kajiyu I didn't notice it! I will take a look in 2 weeks.

btw is there a test added to this module?

@lcnature
Copy link
Copy Markdown
Contributor

Thanks @qihongl ! It looks like we did not add. I will maybe work on it this weekend or next week.

@qihongl
qihongl commented Mar 2, 2021
Copy link
Copy Markdown
Member

@lcnature Ah do you mean there are some new updates that haven't been added to this PR?

@mihaic
mihaic commented Mar 8, 2021
Copy link
Copy Markdown
Member

@lcnature, please remember about this pull request. It seems cl 8000 ose to completion.

Comment thread brainiak/funcalign/srm.py
-------

ll_score : Log-likelihood of test-subject's data
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be better to have an input validation here to make sure all d in data have the same number of samples?

I noticed that later on line 616, you are skipping any d in data that is None. I was wondering if this can be done earlier, such a removing None from the very beginning, or warn the user that there is None in the data list? Feel free to decide what's more user-friendly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah and this function probably want to assert len(data) >= 2

Comment thread brainiak/funcalign/srm.py
Comment on lines +584 to +587
w = []
for subject in range(subjects):
_w = self._update_transform_subject(data[subject], self.s_)
w.append(_w)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not necessary but I think the following is slightly faster

w = [self._update_transform_subject(data[subject], self.s_) for subject in range(subjects)]

Comment thread brainiak/funcalign/srm.py
w.append(_w)

x, mu, rho2, trace_xtx = self._init_structures(data, subjects)
sigma_s = self.sigma_s_
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not necessary but how about just use self.sigma_s_ on line 597?

Comment thread brainiak/funcalign/srm.py

# Invert Sigma_s using Cholesky factorization
(chol_sigma_s, lower_sigma_s) = scipy.linalg.cho_factor(
sigma_s, check_finite=False)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

referred in a comment above

Comment thread brainiak/funcalign/srm.py
wt_invpsi_x = np.zeros((self.features, samples))
trace_xt_invsigma2_x = 0.0
for subject in range(subjects):
if data[subject] is not None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

referred in a comment above

@qihongl qihongl left a comment •
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, I'm not familiar with mpi but I think the code makes sense (in terms of matching the formulas in the paper)!
I think some input validation for data at the beginning of this function would be useful here. And I have 2 other very minor comments. Ah, and a test would be nice. Perhaps a test that computes the score for some toy data?

Thanks!

This branch has not been deployed

No deployments
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.

4 participants

0