Skip to content
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

Improve coding style #106

Merged
merged 17 commits into from
Mar 14, 2016
Merged

Improve coding style #106

merged 17 commits into from
Mar 14, 2016

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Feb 4, 2016

Fixes #79. @mbillingr can you confirm if it is OK to move the cat_trials import outside of the function?

@mbillingr
Copy link
Member

Yes, that is OK. In fact, you could also move the dot_special import outside.

In principle, we could move all local imports and the scipy import outside. The local imports are available anyway, and scipy is a obligatory dependency of scot. Only sklearn is optional and should remain inside the function.

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 5, 2016

@mbillingr I've changed the input argument order in some functions. Could you please check if this is OK, or are we unnecessarily breaking the API? I thought the new order makes more sense.

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 5, 2016

I think I'm going to revert the changed argument order. The statistics example produces different results.

@mbillingr
Copy link
Member

The problem with changing the argument order is that you will never know for sure if you found all the places where the function is used.

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 5, 2016

Agreed. That's why I reverted it.

@mbillingr
Copy link
Member

@cle1109 can you rebase this branch to the current master?

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 10, 2016

@mbillingr could you please check if the changes are OK? If yes, please merge it - I want to avoid blowing up this PR more, because this means a lot of manual merging when rebasing. We can continue to improve the code style in another PR.

mbillingr added a commit that referenced this pull request Mar 14, 2016
@mbillingr mbillingr merged commit d5362bd into scot-dev:master Mar 14, 2016
@cbrnr cbrnr deleted the codestyle branch March 14, 2016 12:30
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