CGATOxford / UMI-tools

Tools for handling Unique Molecular Identifiers in NGS data sets
MIT License
491 stars 190 forks source link

Handle --per-contig grouping with no gene tags #577

Closed akmorrow13 closed 1 year ago

akmorrow13 commented 1 year ago

Replaces https://github.com/CGATOxford/UMI-tools/pull/570, adds unit test

akmorrow13 commented 1 year ago

Can one of the admins verify this patch?

akmorrow13 commented 1 year ago

@TomSmithCGAT can you trigger these tests?

TomSmithCGAT commented 1 year ago

@akmorrow13 - Thanks for the prompt

Please ignore the error. That's because we are using nose, which is incompatible with python 3.10 (https://github.com/nose-devs/nose/issues/1099 & https://github.com/CGATOxford/UMI-tools/pull/546) and we appear to have added py 3.10 to our testing when removing py 3.6 (https://github.com/CGATOxford/UMI-tools/commit/8a7c9c43d7c18fa0d7a82af62782023f7ad164e7).

I'll fix the tests on master and then re-run here.

akmorrow13 commented 1 year ago

@TomSmithCGAT thank you for your help. There was a bug in my test, it should now be fixed.

TomSmithCGAT commented 1 year ago

OK, this is very strange. With the above further commits, I can get the new test to pass with py3.7 & 3.8 but not 3.9 😖

I've can replicate this behaviour locally by switching python versions, so it does seem to be something inherently different between the python versions. I understand dictionaries output order changed recently, but that should all be python>=3.6.

Leave this with me and I'll try and work out what's going on.

TomSmithCGAT commented 1 year ago

🎉 Test passing

@IanSudbery - Whatever the difference is between py3.8 & 3.9, it manifests as differences in the output order. No reason for our tests to fail for different order. Previously, we had a 'Sort' flag for tests where the stdout should be sorted prior to comparison with reference. I've extended this so that it sorts all reference and output files prior to comparison. We really should switch to a more standard test framework when one of has some spare time!

Are you happy for me to merge this?

akmorrow13 commented 1 year ago

Thank you so much for all the work on this @TomSmithCGAT!

IanSudbery commented 1 year ago

Looks good, go ahead.