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

ActiveAx model fitting using MIX framework #1343

Closed
wants to merge 75 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@maryamafzali

maryamafzali commented Sep 29, 2017

ActiveAx model fitting using MIX framework

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2017

I have three high-level comments here, before I have even looked at the code much:

  1. It's awesome to have this as part of Dipy! Thanks for your work on this! I am excited to take a look.

  2. ~3k lines of code is a lot to review in one go! Consider whether there is any way to break this up into a few different related PRs, or a series of PRs to follow each other. I would say that this should probably be broken down into at least 3 different PRs, just based on the number of LOC.

  3. I would recommend using slightly more informative commit messages, so that you communicate more clearly what happened and when. See this excellent blog-post for some guidelines on that: https://chris.beams.io/posts/git-commit/

Maryam Afzali added some commits Oct 20, 2017

Maryam Afzali
Maryam Afzali
Maryam Afzali
Maryam Afzali
Maryam Afzali

@skoudoro skoudoro closed this Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment