Astroua / CARTAvis

Deprecated Repository for CARTA project. Refer to:
https://github.com/cartavis/carta
GNU General Public License v2.0
2 stars 7 forks source link

Fix calculation of intensity locations in quantile-finding function #194

Closed confluence closed 7 years ago

confluence commented 7 years ago

The current code for finding intensity values and their corresponding frame locations for percentiles does not calculate the location values correctly. After copies of the intensity values and their indices have been stored in separate vectors, the values are partially sorted with nth_element, at which point the relationship between the the values and indices is broken. The indices which are subsequently selected from the index vector are completely unrelated to the partially sorted intensities.

In addition to this, the code for calculating the divisor used to convert the absolute index value to a frame on the spectral axis seems to behave incorrectly on images with an additional axis after the spectral axis (e.g. Stokes parameters) -- any axes after the spectral axis are not included in the divisor. But I may have misinterpreted the intended behaviour -- please let me know if the new calculation is not correct.

I have copied and sorted the data as a single vector of index:intensity tuples (using only the intensity value as the key). I also explored an alternative solution in which only the values are sorted and the indices are subsequently looked up by value in the original data, but this solution is faster.

I am not sure what the expected correct behaviour is when the intensity returned for a percentile is found multiple times in different frames, and I would appreciate any guidance regarding this. As far as I know (from my correspondence with @slovelan) these frame values are only used in a unit conversion in the colour map.

low-sky commented 7 years ago

Thanks, @slovelan @confluence

Who would be the right person to hand the conflict before the merge?

confluence commented 7 years ago

Let me have a look -- I think it was introduced when other commits were made to develop in the meantime.

confluence commented 7 years ago

I have synced my fork and fixed the conflict; it should now be possible to merge.

low-sky commented 7 years ago

👍 Thanks a lot.

@ajm-asiaa -- looks like we're good to merge!