Issue_4

Adding the code for Issue_4 as well as the nextflow workflow

Please fix the linter issues (see CI log or just run flake8 locally in your checked out feature branch). Use Google to find out more about what the linter complains about. E.g., for src/PrimingProb.py:63:17: E741 ambiguous variable name 'l', googling E741 ambiguous variable name will bring you here: https://www.flake8rules.com/rules/E741.html

Please proceed in a similar way to make sure the entire CI pipeline passes, then please assign me to review. If you have any specific questions, please ask them during the session or on the forum on ADAM.

added 2 commits

• 557be05b - 1 commit from branch main
• c154c55a - Adding linted version of Issue #4 code

Compare with previous version

added 1 commit

• 75735c39 - feat: adding code for Issue_4

Compare with previous version

Nice, now just the unit tests are left ;-)

Please add a call to the test stage in the CI config that runs this pipeline with some test data so that we know that the code works.

Guess you could remove the 3 from the filename. Git takes care of handling versions.

changed this line in version 13 of the diff

Unit tests are currently still missing so it's difficult to assess if the code works as expected.

changed this line in version 8 of the diff

Please follow the naming conventions for Python: modules, functions and methods should be in snake_case and classes in CamelCase. Also, please don't name things like _final - using version control is sufficient. So better to name this module priming_prob.py.

changed this line in version 8 of the diff

Check this comment about ordering imports: !13 (comment 24993)

changed this line in version 8 of the diff

The way this is implemented, there is no advantage of using a class here over just regular functions. As far as I can see no data is passed between methods. Also, application-level code is interspersed with methods, this is not good practice. I think it's best to refactor in a way that you just use functions and no class.

changed this line in version 8 of the diff

Best is to put the application-level ("main" code) together with the CLI stuff in another module. To avoid conflicts with other people's work, I would put this in a subpackage, e.g., src/priming_prob, then put modules priming_prob.py and cli.py in there. In the latter you would put all the CLI parser and main() code. In the former, you put just the InterPara() and InterProb() functions (but name them inter_para() and inter_prob() rather.

changed this line in version 8 of the diff

requested review from @zavolan

This needs some refactoring as well as unit tests and a test run of the pipeline in the CI. There may also be additional functional comments from @zavolan.

mentioned in merge request !17 (closed)

mentioned in merge request !22 (closed)

mentioned in merge request !23 (closed)

Not a good idea to have hard-coded paths scattered through the code. The output file name should rather be an input parameter or the output should be written to standard out and redirected by the wrapper script (in this case nextflow).

changed this line in version 8 of the diff

This should not be a hard-coded name. The path to the input file is a command-line parameter.

changed this line in version 8 of the diff

It's hard to follow these transformations of one string that use so many distinct variables. Also mycounter is a file handle, not a "counter", you should rename variables in a way that reflects what they are/do.

changed this line in version 8 of the diff

Hard to follow what's going on there. Seems like you would want to do a set intersection here rather than check an entire dictionary for every value in another list.

changed this line in version 8 of the diff

This is an expensive operation, consider only saving the relevant fields from the start.

changed this line in version 8 of the diff