Issue13

Adding the code for the issue 13

Please remove this file manually with rm <file> && git add <file>, then commit and push. You may also want to add a pattern to .gitignore so that these types of files aren't accidentally added in the future (note that this won't affect files that have already been added, hence the manual deletion is necessary).

The same goes for all other files in the .idea/ directory so it's best to add that entire folder as an ignore pattern. Check Google on how to do that.

Please add a module-level docstring in the very first line.

Please place all imports at the top of the module, right after the docstring, but outside of any class or function definitions. Please have a look at PEP 8. Also have a look at this comment: !13 (comment 24993)

In the current implementation, there is no added benefit of having your code defined in a class, as the only defined method is basically a static function and there is no __init__() method or anything like that. I would recommend to just get rid of the class and only keep the function. We can always refactor later in case new code is added and a class actually makes sense

Please use a proper (Google-style) docstring like you do below for the function. Or better (if you decide to get rid of the class), work any important info from here into the function's docstring.

Please add type hints for all your args as well as for the return value, something like:

def my_func(my_arg_1: str, my_arg_2: int = 8) -> str:

^ This function would take one required argument (no default!) of type str and one optional argument (defaults to 8 if not provided) of type int and return a value of type str.

Please keep your testing code separated from your actual implementation. Put all your test files in tests/test_files and write tests that cover all of the conditional branches of your code (you may need to create additional test files for some conditions).

Again, a module-level docstring should be added, followed by all imports, grouped and sorted. Also, it's good practice to define a main() function with all application-level code (i.e., the CLI parsing, calling of your accuracy_estimate() method etc, basically everything between lines 5 and 14), then have the call of the main() function be the only thing you do in the `if name should be defined at the top of the module, and

Instead of importing the module, it's usually better (more performant and more explicit) to import only what you really need, so better say:

from accuracy_estimate.AccuracyEstimate import accuracy_estimate

Or, if you remove the class, this becomes a little simpler:

from accuracy_estimate import accuracy_estimate

Please move this file and the next to tests/test_files

What input values?

You only need these once, in your call in line 14, so there's little use of assigning these here. Instead you can just call the function like this (if you import the function/method directly):

accuracy_estimate(
    input_file=args.input_file,
    simulation_file=args.simulation_file,
)

This is easier to read and modify later on.

Also, please note that input_file is not a very descriptive file name. All arguments to a function are basically inputs, and whether something is a file can go in the description of the arguments. Better to find some more descriptive names for the CLI and function arguments.

The function doesn't seem to return what the docstring says it does. In fact there is no return statement here at all, so it will return None. Given that we want to use the script in a workflow, we cannot have interactive parts, like opening a plot, which would require the user to interact with it or close it. So this should be reimplemented such that the plot is flushed to a file and the file path returned instead.

Remove this line.

Apart from the minor comments above, the code should be unit-tested and a one-step Nextflow subworkflow written (see e.g., !10 (closed) and !16 (closed)) to show that this actually works as expected.

mentioned in merge request !23 (closed)

mentioned in merge request !21 (closed)

closed