Skip to content

Conversation

@lcnature
Copy link
Contributor

This PR is to fix the issue of #253

Copy link
Member

@mihaic mihaic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @lcnature! I made a couple of minor comments.

Regarding the test failure, see PR #403.


__all__ = [
"get_sigma", "gmm_1d_distribution", "get_next_sample",
"fmin"
Copy link
Member

Choose a reason for hiding this comment

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

I think only fmin should be listed here. I made issue #402 about renaming the others, but we could already list only what is necessary in __all__. What do you think?

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 am fine with that. I did not read into detail of what other commands do.

"ConditionSpec",
"SingleConditionSpec",
"mask_image",
"MaskedMultiSubjectData",
Copy link
Member

Choose a reason for hiding this comment

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

I was trying to ordered __all__ alphabetically.

"mask_image",
"MaskedMultiSubjectData",
"multimask_images",
"mask_images"
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comma after the last element as well in multi-line lists, to minimize the changes made later on. See PEP 8:
https://www.python.org/dev/peps/pep-0008/#when-to-use-trailing-commas

brainiak/isc.py Outdated
"permute_two_sample_iscs",
"permutation_isc",
"timeshift_isc",
"phaseshift_isc"
Copy link
Member

Choose a reason for hiding this comment

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

@snastase, are all these supposed to be used outside BrainIAK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I fixed up all and prepended underscores in the appropriate spots for isc.py

"ecdf",
"p_from_null"
"p_from_null",
"compute_p_from_null_distribution"
Copy link
Member

Choose a reason for hiding this comment

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

Could you please insert the new ones alphabetically?

@mihaic
Copy link
Member

mihaic commented Jan 14, 2019

See PR #408 for a fix for the current build failure.

@mihaic mihaic merged commit 5f7628c into brainiak:master Jan 14, 2019
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.

3 participants