8000 Mostly bug fixing for brsa by lcnature · Pull Request #144 · brainiak/brainiak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Mostly bug fixing for brsa #144

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

Merged
merged 39 commits into from
Oct 31, 2016
Merged

Mostly bug fixing for brsa #144

merged 39 commits into from
Oct 31, 2016

Conversation

lcnature
Copy link
Contributor

And some typo corrections.
The fitV and fitU steps in the _fit_diagV_noGP and _fit_diagV_GP functions are swapped when debugging. But I think the order does not matter so I did not swap them back.
Update example notebook to mention that z-scoring data is necessary.

lcnature and others added 30 commits September 2, 2016 10:12
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
…eals with shared time series not explained by design matrix
Fixed some bugs in utils.ReadDesign
Renamed epsilon to eta in consistence with the paper
Expanded the example a bit
Other minor changes
…eals with shared time series not explained by design matrix
 Changes to be committed:
	modified:   brainiak/reprsimil/brsa.py
	modified:   brainiak/utils/utils.py
	modified:   examples/reprsimil/brsa_representational_similarity_estimate_example.ipynb
	modified:   tests/reprsimil/test_brsa.py
	modified:   tests/utils/test_utils.py
logger.info('You did not provide time seres of no interest '

logger.info('You did not provide time series of no interest '

Copy link
Member

Choose a reason for hiding this comment

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

You probably do not want these empty lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! fixed

LL, LAMBDA_i, LAMBDA, YTAcorrXL_LAMBDA, sigma2 \
= self._calc_LL(rho1, LTXTAcorrXL, LTXTAcorrY, YTAcorrY,
X0TAX0, SNR2, n_V, n_T, n_run, rank, n_base)
except:
Copy link
Member

Choose a reason for hiding this comment

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

You must specify a particular exception and you must add that exception to the docstring of the function in a section called "Raises".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-->

OK I removed the try/except

logger.debug('YTAcorrY: {}'.format(YTAcorrY))
logger.debug('LTXTAcorrY: {}'.format(LTXTAcorrY))
logger.debug('YTAcorrXL_LAMBDA: {}'.format(YTAcorrXL_LAMBDA))
logger.debug('SNR2: {}'.format(SNR2))
Copy link
Member

Choose a reason for hiding this comment

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

Have you thought if warnings.warn() or logger.warning are appropriate here?

Copy link
Member

Choose a reason for hiding this comment

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

Here are some guidelines for when to use what:
https://docs.python.org/3/howto/logging.html#when-to-use-logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to logger.warning

@@ -7,7 +7,7 @@
},
"source": [
"# This demo shows how to use the Bayesian Representational Similarity Analysis method in brainiak with a simulated dataset.\n",
"Questions can be directed to mcai [ at ] princeton [ dot ] edu"
"*Feedbacks and questions are welcome. Please direct them to mcai [ at ] princeton [ dot ] edu"
Copy link
Member

Choose a reason for hiding this comment

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

Not really what the change is about, but do you mind removing this line? We are already instructing users to open GitHub issues for BrainIAK or contact us on Gitter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!

@mihaic mihaic merged commit a08ac6a into brainiak:master Oct 31, 2016
@lcnature lcnature deleted the spatial_corr branch August 10, 2017 21:59
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.

2 participants
0