Skip to content
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

Better warnings for command line arguments that exist for compatibility #5

Open
moldach opened this issue Apr 23, 2020 · 6 comments
Open
Labels
good first issue Good for newcomers

Comments

@moldach
Copy link

moldach commented Apr 23, 2020

Tried to run falco with 1 and 6 cores but I didn't see any difference.

Basic benchmark

Job Wall-clock time: 01:08:53
Memory Utilized: 87.13 MB
PE FASTQ (17G for R1, 22G for R2)
@guilhermesena1
Copy link
Collaborator

Hi Matthew,

At the moment falco does not allow multithreading. We have discussed making it multithread during development, but decided to not make it so because the slowest part of the program are actually the IO, so reading a batch of sequences and splitting/merging the QC operations would not decrease the run time by much. As far as I know other QC tools like fastqc usually multithread in the case where you have multiple FastQC files and want to process them independently, but this is equivalent to running separate jobs.

@pbiology
Copy link

This seem reasonable. However, the help documentation has the option -t/--threads, which makes it sound like this feature exists

@jpcartailler
Copy link

Note to anyone reading this thread, the help (online here and in -h CLI option) does state (currently set for compatibility with fastqc. Not yet supported!). From reading the paper (https://f1000research.com/articles/8-1874), this is by design:

We use the same set of command-line arguments, configuration file names, and input file formats as FastQC. We also produce the same plain text format output, and the same report structure, allowing users to take advantage of improved speed without adjusting to different program behaviors.

@jpcartailler
Copy link

FWIW, the help (online in README and in -h CLI option) does state (currently set for compatibility with fastqc. Not yet supported!). From reading the paper (https://f1000research.com/articles/8-1874), this is by design:

We use the same set of command-line arguments, configuration file names, and input file formats as FastQC. We also produce the same plain text format output, and the same report structure, allowing users to take advantage of improved speed without adjusting to different program behaviors.

@andrewdavidsmith andrewdavidsmith changed the title Can you run falco in parallel? Better warnings for command line arguments that exist for compatibility Sep 3, 2022
@andrewdavidsmith
Copy link
Collaborator

andrewdavidsmith commented Sep 3, 2022

The "better" warnings means ensuring they are uniform, with the exact same language for each such argument, and also a bit more prominent in the help/docs. Closing this issue involves determining what kind of changes to the help output would be acceptable, collecting those arguments that are present only for compatibility, and adjusting the text reported. Likely very easy issue.

Obviously this would require updating the README and anything that mirrors whatever help text would be changed within the code.

@andrewdavidsmith andrewdavidsmith added the good first issue Good for newcomers label Sep 3, 2022
@guilhermesena1
Copy link
Collaborator

guilhermesena1 commented Sep 11, 2022

So I worked a bit on this issue and rewrote the description of falco flags. Each flag for which the behavior differs from FastQC has the following brackets before them:

[IGNORED BY FALCO]: The flag does nothing, and never will because it is obsolete/is not necessary the way falco works.
[NOT YET IMPLEMENTED IN FALCO]: The flag does nothing, but eventually will
[Falco only]: A functionality that does not exist in FastQC.

I also used the convention of keeping double dashes for FastQC, but single dashes for falco so there is no collision in behavior (although double dashes on falco's arguments would be fine too)

These are reflected in commit 159e7f3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants