search

bcgsc / physlr

:chains: Construct a Physical Map from Linked Reads
GNU General Public License v3.0
18 stars 8 forks source link

btllib #186

Closed aafshinfard closed 1 year ago

aafshinfard commented 1 year ago

Tested using btllib v1.4.4 Tested on NA12878 stLFR data and compared to the master branch

Branch Max NG25 NG50 NG75 Min Count Sum
Master 128,316,698 86,871,355 57,146,846 29,925,152 165,302 98 2,803,687,684
btllib 111,001,009 86,583,412 51,307,122 28,406,893 42,228 96 2,785,382,854
aafshinfard commented 1 year ago

Did some search and trials to fix the failing CI tests, specifically the clang-tidy compiler errors and warnings-as-errors. First issue: it still cannot see the btllib lib; second issue: we are moving from clang v7 to clang v15 and there are many warnings-as-errors we need to fix... I can workaround this by excluding the .clang-tidy file to skip extra checks for now and fix them later in another PR. Also tried fixing with clang-tidy --fix-errors but it messed things up.

aafshinfard commented 1 year ago

Just removed two dependencies from Environments.yml and things are working alright. @lcoombe ready for reviews. Thanks.

lcoombe commented 1 year ago

Thanks for all this @aafshinfard ! My last question is just about the .clang-tidy being renamed to cland-tidy.BAK - can you just remind me why that's needed?

aafshinfard commented 1 year ago

Thanks for all this @aafshinfard ! My last question is just about the .clang-tidy being renamed to cland-tidy.BAK - can you just remind me why that's needed?

I just did not want to spent too much time making those additional syntax perfections it required, so wanted to disable it but also keep something there for future when I have time to enable it and make those changes... but I think it'd make more sense to just remove this file and keep the repo more simple. We can always add another one if we need to.

lcoombe commented 1 year ago

Thanks for all this @aafshinfard ! My last question is just about the .clang-tidy being renamed to cland-tidy.BAK - can you just remind me why that's needed?

I just did not want to spent too much time making those additional syntax perfections it required, so wanted to disable it but also keep something there for future when I have time to enable it and make those changes... but I think it'd make more sense to just remove this file and keep the repo more simple. We can always add another one if we need to.

Yes I agree - let's just remove it for now (it'll be available for us to re-add in the future from the repo history anyway)

aafshinfard commented 1 year ago

Thanks Amirhossein!

Thank youuu for the review!