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

Various small fixes for v1.0.0a5 #79

Merged
merged 16 commits into from
Jan 26, 2023
Merged

Various small fixes for v1.0.0a5 #79

merged 16 commits into from
Jan 26, 2023

Conversation

emarinier
Copy link
Collaborator

@emarinier emarinier commented Nov 10, 2022

  • Modified updatedb's wget to write less to stdout.
  • Made sure quast uses the specified contig size.
  • "continue" -> "break" when considering mash command size
  • Changes the directory mash runs from to reduce to length of file paths.

@emarinier emarinier added the bug Something isn't working label Nov 10, 2022
@emarinier emarinier self-assigned this Nov 10, 2022
@emarinier emarinier requested a review from ericenns November 10, 2022 17:37
@emarinier
Copy link
Collaborator Author

emarinier commented Dec 21, 2022

@ericenns The solution for using shorter relative paths instead of longer absolute paths involves finding the common path of all input files and working from the directory of the common path. There's a bit of extra logic to handle when there's only 1 input file or multiple input files in completely different directories.

@emarinier emarinier requested a review from apetkau January 9, 2023 19:05
Copy link

@apetkau apetkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @emarinier, awesome fixes 😄. I did have some comments I left.

Also, if I wanted to run this to test, would I just use proksee evaluate on some fasta file with many contigs?

Copy link

@apetkau apetkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for all the changes. They all look amazing 😄

When I went to test it though, I got an error. Running like proksee evaluate -o out contigs.fasta gives me the following exception:

Traceback (most recent call last):
  File "~/miniconda3/envs/proksee/bin/proksee", line 8, in <module>
    sys.exit(cli())
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/proksee/commands/cmd_evaluate.py", line 62, in cli
    evaluate(contigs, output, mash_database_path, species, min_contig_length)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/proksee/commands/cmd_evaluate.py", line 93, in evaluate
    mash_database_path, id_mapping_filename, species_name)
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/proksee/utilities.py", line 62, in determine_species
    species_list = species_estimator.estimate_major_species()
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/proksee/species_estimator.py", line 116, in estimate_major_species
    mash_filename = self.run_mash()
  File "~/miniconda3/envs/proksee/lib/python3.7/site-packages/proksee/species_estimator.py", line 211, in run_mash
    stdout=unsorted_output_file, stderr=NULL)
  File "~/miniconda3/envs/proksee/lib/python3.7/subprocess.py", line 488, in run
    with Popen(*popenargs, **kwargs) as process:
  File "~/miniconda3/envs/proksee/lib/python3.7/subprocess.py", line 800, in __init__
    restore_signals, start_new_session)
  File "~/miniconda3/envs/proksee/lib/python3.7/subprocess.py", line 1551, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '': ''

This same command works when using the version of proksee from the develop branch.

@emarinier
Copy link
Collaborator Author

Thanks Aaron. Good catch. It turns out I needed to take the absolute path of any file I try to take the directory name from, because a relative path without a directory (ex: "contigs.fasta") would return an empty string for the dirname and then subprocess would try to run the process from an empty string directory.

Copy link

@apetkau apetkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. Thanks so much Eric.

@apetkau apetkau self-requested a review January 19, 2023 18:25
Copy link

@apetkau apetkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made one other comment about parallel mash screen (just writing this here again so it's part of a review).

@emarinier
Copy link
Collaborator Author

I made one other comment about parallel mash screen (just writing this here again so it's part of a review).

Added in d1071ce

Copy link

@apetkau apetkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks so much 😄. Sorry about all the change requests and delays in reviewing. You've done a great job with this.

@emarinier emarinier merged commit 9612458 into develop Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants