yeatmanlab / AFQ-Browser

Browser-based visualization tools for AFQ results
BSD 3-Clause "New" or "Revised" License
33 stars 15 forks source link

enh: added tract extent on brushing #175

Closed akeshavan closed 7 years ago

akeshavan commented 7 years ago

resolves #165

arokem commented 7 years ago

Two comments:

jyeatman commented 7 years ago

Agreed - cast as integer

I'm fine with dash or comma

On Fri, Nov 3, 2017 at 1:38 PM, Ariel Rokem notifications@github.com wrote:

Two comments:

  • I would prefer a comma to a dash in between them (my brain sees this as a negative sign, and gets confused)
  • I would cast to integer: I think that it's more accurate given that these are discrete nodes (does that make sense to you @jyeatman https://github.com/jyeatman?)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ-Browser/pull/175#issuecomment-341820680, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsYys7fahm_h7dL1bzKsejWgz55mdRqks5sy3msgaJpZM4QQUN0 .

akeshavan commented 7 years ago

done - screen shot 2017-11-03 at 2 39 46 pm

jyeatman commented 7 years ago

Is this ready to be merged? If so I will test in a few cases and then merge if there are no issues

richford commented 7 years ago

LGTM

arokem commented 7 years ago

Oops. Sorry - I figured we were all in agreement, and then saw the "test in a few cases". I guess we can fix up later if needed :-/

jyeatman commented 7 years ago

All good

On Mon, Nov 6, 2017 at 11:09 AM, Ariel Rokem notifications@github.com wrote:

Oops. Sorry - I figured we were all in agreement, and then saw the "test in a few cases". I guess we can fix up later if needed :-/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ-Browser/pull/175#issuecomment-342252804, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsYytLmLcINPi2J2zPdCiszp5Ob7Gvgks5sz1lQgaJpZM4QQUN0 .