higlass / clodius

Clodius is a tool for breaking up large data sets into smaller tiles that can subsequently be displayed using an appropriate viewer.
MIT License
38 stars 21 forks source link

cooler >0.9.0 support #150

Closed sergpolly closed 1 year ago

sergpolly commented 1 year ago

there is a little conundrum in the higlass-python/clodius/cooler/cooltoos interoperability now:

what is incompatible between cooler 0.9.1 and clodius at the moment ? is it just tiles/cooler.py ? I'm willing to look at it - just trying to narrow down the issue ...

sergpolly commented 1 year ago

anecdotally - simply removing max_chunk=np.inf argument from clr.matrix() call: https://github.com/higlass/clodius/blob/dceb4bcb225e31f6669cc8be22485b8e56e2c082/clodius/tiles/cooler.py#L98 fixes tests in tests/tiles/cooler.py

todo - test higlass-python with such modified clodius for serving local coolers

manzt commented 1 year ago

I know @nvictus was prototyping a new tile fetcher for clodius > 0.9. Maybe that’s in a state he can share?

sergpolly commented 1 year ago

hm - so far I can confirm that, the simple fix was enough ... and higlass-python is happily serving me a local cooler from within open2c examples : Screenshot from 2023-04-20 12-30-16

manzt commented 1 year ago

It's not clear to me what this used to do because the max_chunk parameter isn't included in the old docstring.

https://github.com/open2c/cooler/blob/ac779db1899a1ba295a0a47db3de5873aaa47ac1/cooler/api.py#L301-L341

I guess we could either choose to remove the parameter or have the newer cooler API accept **kwargs as a final catch all for kwargs. My feeling is to just remove max_chunk as you have, and pin cooler>=0.9.0 in the pyproject.toml for clodius.

sergpolly commented 1 year ago

here is what I found in the old version: Screenshot from 2023-04-20 19-40-27 CSRReader is where matrix method uses max_chunk parameter - so I think it would be fair to say that it used to control performance/memory-footprint tradeoff ...

It is unclear however what happened to this "contol knob" in the 0.9.0... I know that 0.9.0 was a major refactoring of the query engine... Wait... - the new API (e.g. matrix method) has chunksize (!) which remains undocumented, - perhaps it does the similar thing as max_chunk before

nvictus commented 1 year ago

Yes, removing max_chunk should do the trick until the new tile fetcher is merged.

Phlya commented 1 year ago

Just wanted to upvote this issue, especially since the fix appears to be simple.