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

Add a score function for SRM#478
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 close 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!

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