-
Notifications
You must be signed in to change notification settings - Fork 954
feat(merquryfk/merquryfk): use topic versions and emit all images in single channel #9672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
charles-plessy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me but have some minor questions. Also, I do not have experience with this module, so I recommend to leave some time to others to comment before merging.
|
I tried bumping to the latest 1.1.3 release to see if it fixes the error with trio inputs, but no dice. So I'm happy to leave that disabled for now and to see if we can get the issue fixed later. edit: I have found the bug and created a PR to fix it upstream, also: thegenemyers/MERQURY.FK#20 |
|
OK, I've spoken with the author and he is has written and will tag a fix for this bug. So I'll hold off on merging this PR until that's sorted and the new version is live, then incorporate that so we have a working module. |
|
OK, almost there - this module needs some real trio test data as the test fails due to a floating point error. I'll add this to test-datasets, then we should be good to go. |
|
OK, I think this is good to go! @mashehu, might you be able to force-merge again? 🙏 This one also suffers from the |
|
👍🏻 will wait for the tests to pass though |
|
@mashehu And tests passing here 🙏 |

A few changes here:
edit: this appears to be a bug in merquryfk, it refers to a wrong file name internally. Not sure if it's been fixed as there hasn't been a new version tagged in a while...
edit2: Yes, this is a bug in the tagged version of merquryFK - I can run the command in
.command.shfine with the latest commit. I'll endeavour to get Gene to tag a release, but this might take some time or not be possible.edit3: MerquryFK bumped to 1.2 and added a trio test dataset. Now the tests pass and the tool works correctly :)
PR checklist
Closes #XXX
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda