neurodata / r-mgc

R package for MGC code
Apache License 2.0
9 stars 10 forks source link

satish comments #34

Closed ebridge2 closed 5 years ago

ebridge2 commented 5 years ago

@ebridge - note the change in mgc, let's update R code too?

@satish - thanks for keeping us honest! this is why we need tests in our code, and why the python code will soon be better than both R and MATLAB (python will have extensive testing, and continuous integration :)

On Sun, Nov 11, 2018 at 6:00 PM Cencheng Shen shenc@udel.edu wrote: Satish,

Thank you for bringing this to my attention -- that was an unexpected bug from the previous scale change!

I just fixed it and pushed the change, it should work as intended now. Let me know if you notice other issues!

Best, Cencheng

On Sun, Nov 11, 2018 at 5:38 PM Satish Palaniappan spalani2@jhu.edu wrote: Okay, I do understand that we are talking about the case where, X & Y are NOT dependent and the optimal scale should be [0,0] then. But the code change made here: https://github.com/neurodata/mgc-matlab/blob/master/Code/MGCSmoothing.m#L25, not only does this, but also will also return [0, 0] if the data is linear and IS correlated, instead of the optimal scale. Because I tried the same change in the Python version and this is what the behavior was. Maybe something else is going wrong.

Satish Palaniappan,

Graduate Student (Masters in Computer Science),

Johns Hopkins University.

GitHub | LinkedIn | Resume/CV | Google Scholar

From: joshua vogelstein jovo@jhu.edu Date: Sunday, November 11, 2018 at 5:30 PM To: Satish Palaniappan spalani2@jhu.edu, Cencheng Shen shenc@udel.edu Cc: Junhao Xiong jxiong3@jhu.edu Subject: Re: quick question

you are wrong.

we are concerned with the case that X & Y are NOT dependent,

and what the code outputs as the optical scale in that scenario.

we think it should be [0,0].

make sense?

On Sun, Nov 11, 2018 at 4:41 PM Satish Palaniappan spalani2@jhu.edu wrote:

Hi Jovo!

Sure, I will make the change in mgcpy.

But, I have a question, if both the data are linear and are highly correlated, then I was under an understanding that, the optimal scale should default to the global scale, but how will, making this change: https://github.com/neurodata/mgc-matlab/blob/master/Code/MGCSmoothing.m#L25, do that for us. This change will return just [0, 0] as the optimal scale in the case I just told. Correct me if I am wrong.

Thanks,

Satish Palaniappan,

Graduate Student (Masters in Computer Science),

Johns Hopkins University.

GitHub | LinkedIn | Resume/CV | Google Scholar

From: Junhao Xiong jxiong3@jhu.edu Date: Friday, November 9, 2018 at 11:04 AM To: Satish Palaniappan spalani2@jhu.edu Cc: jovo Vogelstein jovo@jhu.edu Subject: Fw: quick question

Forwarded to Satish to see how we can update this in our code.

Best,

Bear

From: joshua vogelstein jovo@jhu.edu Sent: Friday, November 9, 2018 9:45 AM To: Cencheng Shen Cc: Michael Milham; Eric Bridgeford; Junhao Xiong Subject: Re: quick question

ebridge2 commented 5 years ago

R: https://github.com/neurodata/mgc/blob/034795d039d85f29a31d0353a33054ebd739df11/R/MGCDistTransform.R#L73 mgc-matlab: https://github.com/neurodata/MGC-paper/blob/953019aafdfebead6c5cdd92442809638ce27936/Code/MGC/MGCDistTransform.m#L69