balbasty / nitorch

Neuroimaging in PyTorch
Other
83 stars 14 forks source link

Fix atlas align #63

Closed brudfors closed 2 years ago

brudfors commented 2 years ago

Regarding the two fixes, this is the error relating to giving int16 to the cutoff function:

image

and this is the error relating to having a CPU device when adding noise:

image

brudfors commented 2 years ago

@balbasty it seems as if the cutoff function should work on both numpy arrays and torch tensors, so that .float() errors when invoked on an nd.array. What is the best way of getting it working for both numpy and torch data?

balbasty commented 2 years ago

How about converting to float around line 225 of volutils (inside the torch branch of the cutoff function). You may need to then take the floor/ceil of the returned percentile so that they can be used to clamp the (original) integer type dat.

Of course it triggers a full copy, which is not optimal. But I don't have a much better idea right now.

balbasty commented 2 years ago

Also, don't just call float (that would trigger a conversion when the input data is double, which we don't want). You can do something like "if not dat.dtype.is_floating_point: convert..."

brudfors commented 2 years ago

@balbasty, I moved the float conversion into the cutoff function, and only apply it it data is not float. For it to work with io.mapping, it is not necessary to round the percentiles, as the imlicit coversion from rhs float to lhs int takes care of that. But maybe cutoff should be made to return the same datatype as it is given? At the moment, if float is applied (the data is int), then the output will be changed from int float.

balbasty commented 2 years ago

Yes, I think it should return the same datatype. And if possible, not through "convert of convert". We want to keep the original integer-typed data somewhere and clip that one. That's why I thought we needed to convert the percentiles, so they can be applied to the integer-type data. I guess it should be min <- int(floor(min)) and max <- int(ceil(max))

brudfors commented 2 years ago

The problem is that this call errors:

pct = utils.quantile(dat, cutoff)

if dat is not float, which mean we would have to hold both the float and int data in memory.

Maybe better to modify inside utils.quantile then?

balbasty commented 2 years ago

Something like:

dat_for_quantile = dat
if not dat.dtype.is_floating_point:
    dat_for_quantile = dat_for_quantile.float()

cutoff = [val/100 for val in cutoff]
pct = utils.quantile(dat_for_quantile, cutoff)

if len(pct) == 1:
    mn, mx = None, pct[0]
else:
    mn, mx = pct[0], pct[1]
if not dat.dtype.is_floating_point:
    mx = mx.ceil()
    if mn is not None:
        mn = mn.floor()

mx = mx.to(dat.dtype)
if mn is not None:
    mn = mn.to(dat.dtype)
dat.clamp_(mn, mx)

return dat
balbasty commented 2 years ago

The problem is that this call errors:

pct = utils.quantile(dat, cutoff)

if dat is not float, which mean we would have to hold both the float and int data in memory.

Maybe better to modify inside utils.quantile then?

Yes we do need to hold them both in memory. I think that even if we modify quantile, there will be two copies at some point. I don't really see the alternative.

brudfors commented 2 years ago

Agree, but maybe it is nicer to have the implementation hidden inside utils.quantile? Seeing now we are only modifying its output in cutoff?

brudfors commented 2 years ago

@balbasty, updated as you suggested.

brudfors commented 2 years ago

@balbasty, I will let you do the merge.