Potential bug in filling in NaNs in snipping

[Phlya]

I think the way we (or I?) implemented filling in NaNs in snipping can cause wrong assignment of NaNs with multiprocessing, or when multiple selections have been made before snipping.

cooltools/cooltools/api/snipping.py

Lines 353 to 359 in c610f43

	if self.clr_weight_name:
	self._isnan1 = np.isnan(
	self.clr.bins()[self.clr_weight_name].fetch(region1_coords).values
	)
	self._isnan2 = np.isnan(
	self.clr.bins()[self.clr_weight_name].fetch(region2_coords).values
	)

This block would overwrite the self._isnan1/2 arrays for each selection, while it should be stored in a dict for each region, perhaps.

[agalitsyna]

Yeah, looks like a potential problem. There are two maps: one in pileup_legacy and one in _pileup. The outer parallelized map is propagated to the first function, but not the second. Thus, regions are processed in parallel, but the individual snippets are cut out sequentially.

What if you simply change the order of them? I mean, you can propagate map parameter to the _pileup and use inside while keeping the processing of regions sequential, which will guarantee that the snipper object attributes are not changed on the fly? Have you tried it?

[Phlya]

I have not. If that approach has the same speed, it would be better because it won't require more memory to use multiple cores!
But I think we should fix this issue independently of the maps: in case someone wants to use the snippers directly...

[Phlya]

The apparent issue is at the moment a non-issue because of the Global Interpreter Lock: python copies every object from the main process to child processes. So the _isnan1/2 attributes are set independently within the child processes, hence they don't have a possibility of overwriting or using wrong isnans. Should python ever drop the GIL and stop copying all object to child processes (i.e. use just threading), this will become an issue, that can be solved by pr #339 .

[nvictus]

Then it should be commented that these operations are not thread-safe, in case someone ever decides to switch to multithreading.