BIC-MNI / minc-tools

Basic minc-tools from former minc repository
Other
30 stars 25 forks source link

minccalc range bug #18

Closed andrewjanke closed 9 years ago

andrewjanke commented 9 years ago

From minc-users (Alex Zijdenbos) - a minimal test dataset was never provided so I can't vouch as to its veracity. If it does turn out to be true my first guess is that it is related to a decision of Peter Neelins to convert the symbol table from local to global when I originally submitted minccalc. This made a few assumptions about the way the let calculus was implemented that may turn out to be always correct.

Peter replied to this on minc-users somewhere IIRC, but I can't find it now.


I think something that I think can be legitimately called a bug in minccalc. It seems that the for construct somehow modifies a variable that determines its range. Specifically, this construct:

n=len(A); for {i in [0,len(A))} { stuff } n;

results in a volume that contains the number of input volumes at every voxel (as one would expect). However, this one:

n=len(A); for {i in [0:n)} { stuff } n;

returns a volume that contains len(A) - 1

So somehow, the for construct modifies n?!

andrewjanke commented 9 years ago

Peters response was in another private thread, here's hoping he's not adverse to me posting it here.


I took a quick look at the code, but I haven't played with a debugger, so my hypothesizing might be all wrong. If I'm reading things correctly, gen_range() calls eval_scalar(), and then increments the vals field of the returned scalar. eval_scalar() in general creates a new scalar for the result, except for the optimisation to re-use the first arg if no one is using it. And except for the special cases: one of these is NODETYPE_IDENT which just increments the reference count. (BTW, I'm looking at minc 1.1 code, but I am assuming that this bit has not changed too much.) I think that this is the trouble spot since we are modifying the returned scalar (not allowed, given the reference counting thing).

I believe that a simple fix would be to make a copy of the start and stop scalars in gen_range(). That way the increment cannot affect the original symbol. Just be sure to free both the scalars returned by eval_scalar() and the copies. Even if this solution is a bit hackish, if it works you will know that we are on the right track.

Of course, this is me spouting off ideas after 10 minutes of code inspection (and ten years of memory decay), so I might be completely off. But give it a shot.

rdvincent commented 9 years ago

I think Peter's interpretation is correct. Interestingly, a similar bug occurs if the lower bound of a range is specified with a named value, e.g.

m=0;
n=len(A);
for {i in (m:n)} { stuff }
m;

This will yield a volume with all voxels set to 1, because the value of m will be incremented in gen_range(). I am thinking about how best to fix this, but Peter's suggestion definitely works.

rdvincent commented 9 years ago

@andrewjanke Please see commit f1ac182cac39309263d3d062f4edc5960ba0406f for a possible fix. It may be more intrusive than necessary, but it returns a copy of the scalar value of a symbol.

In fact, if you check out the last three commits, I also added a quick test script for minccalc as well as fixed a minor memory leak.

Let me know what you think...

andrewjanke commented 9 years ago

Works for me. The copy of the scalar should only relate to the current node so I don't see this as a large overhead.

Thanks

a

On 21 November 2014 08:01, Robert D Vincent notifications@github.com wrote:

@andrewjanke https://github.com/andrewjanke Please see commit f1ac182 https://github.com/BIC-MNI/minc-tools/commit/f1ac182cac39309263d3d062f4edc5960ba0406f for a possible fix. It may be more intrusive than necessary, but it returns a copy of the scalar value of a symbol.

In fact, if you check out the last three commits, I also added a quick test script for minccalc as well as fixed a minor memory leak.

Let me know what you think...

Reply to this email directly or view it on GitHub https://github.com/BIC-MNI/minc-tools/issues/18#issuecomment-63888189.