search

aidenlab / straw

Extract data quickly from Juicebox via straw
MIT License
62 stars 36 forks source link

Incompatible function signature introduced by a recent commit (0e9cfa) #60

Closed haizi-zh closed 3 years ago

haizi-zh commented 3 years ago

Describe the bug

A recent commit (https://github.com/aidenlab/straw/commit/0e9cfa51426a718b668ebe63237e1a76b53bab61) changes function signatures, by adding 'matrix' parameter of 'observed' or 'oe': See the git blame record: https://github.com/aidenlab/straw/blame/90367afab2f4142860f53567a6cb5a1d26a007a9/R/R/RcppExports.R#L26

However, since matrix is a new argument and is put as the first without a default value, this breaks other packages. For example, in the HiCRep package: https://github.com/TaoYang-dev/hicrep/blob/e485dfa71dc98cadbbda70424084e85a4a94e3b0/R/hic2mat.R#L23

For now, since this commit has been released for a while, and some other people's code may depend on this new signature, there's no way going back. I suggest you update your package's version number: https://github.com/aidenlab/straw/blob/90367afab2f4142860f53567a6cb5a1d26a007a9/R/DESCRIPTION#L3

So that other packages can at least update their dependency specification, and make sure the correct version of strawr is installed.

Thanks!

nchernia commented 3 years ago

Thanks. @sa501428 we should take care of this ASAP

sa501428 commented 3 years ago

Thank you for this catch; we will be fixing this bug so that it remains backward compatible and that the old usage is still supported.

sa501428 commented 3 years ago

@haizi-zh can you try the latest version that was just merged? It should be backward compatible now / please let us know if the issue is still happening.

haizi-zh commented 3 years ago

@sa501428 Sorry for the late reply. I've just checked the latest commit (https://github.com/aidenlab/straw/commit/40459914ee616f0d3ca2f670d2ba5d4923f688af), and it seems you haven't touched the problem yet.

The problem is simple. Please see the git blame below: https://github.com/aidenlab/straw/blame/40459914ee616f0d3ca2f670d2ba5d4923f688af/R/R/RcppExports.R#L26

The original R function signature was records = straw(norm, fname, chr1loc, chr2loc, unit, binsize);, and in that commit you accidentally changed it to records = straw(matrix, norm, fname, chr1loc, chr2loc, unit, binsize);. Since you introduced a new argument matrix at the beginning of the function call signature, this makes some codes broken.

Unfortunately, in the latest commit, I can see that the function signature remains the same.

sa501428 commented 3 years ago

@haizi-zh this should be resolved by https://github.com/aidenlab/straw/pull/66

haizi-zh commented 3 years ago

@sa501428 Thanks, that's working now.