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

Function to write GOC scores to a file #485

Open
wants to merge 1 commit into
base: feature/gene_tree_benchmark_new
Choose a base branch
from

Conversation

ipilizota
Copy link
Member

@ipilizota ipilizota commented Jul 28, 2022

Description

A function to write GOC scores to a file.

Related JIRA tickets:

Overview of changes

New function and corresponding tests.

Testing

pytest

Notes

Not sure if these OSErrors should be tested at all. For some reason it rings a bell that the conclusion from one of the Dev Round Tables was no - because they originate from the system.


PR review checklist

  • Is the PR against an appropriate branch?
  • Does the code adhere to coding guidelines?
  • Does the code do what it claims to do?
  • Is the code readable? Is it appropriately documented?
  • Is the logic in the correct place?
  • Was the code tested appropriately? Are there unit tests? Are unit tests self-contained and non-redundant?
  • Did Travis CI pass for the code in the PR? Is Codecov acceptable based on the included/updated unit tests?
  • Will the new code fail gracefully?
  • Does the code follow good practice for writing performant code (e.g. using a database transaction rather than repeated queries outside of a transaction)?
  • Does it bring in an unnecessary dependency?
  • If you are reviewing a new analysis, is it future-proof and pluggable?
  • Does the PR meet agile guidelines?

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #485 (6cb88aa) into feature/gene_tree_benchmark_new (4c7380d) will decrease coverage by 0.86%.
The diff coverage is n/a.

❗ Current head 6cb88aa differs from pull request most recent head 4f20abd. Consider uploading reports for the commit 4f20abd to get more accurate results

@@                         Coverage Diff                         @@
##           feature/gene_tree_benchmark_new     #485      +/-   ##
===================================================================
- Coverage                            61.37%   60.50%   -0.87%     
===================================================================
  Files                                  206      195      -11     
  Lines                                22877    22076     -801     
  Branches                              3576     3576              
===================================================================
- Hits                                 14040    13357     -683     
+ Misses                                7631     7513     -118     
  Partials                              1206     1206              
Impacted Files Coverage Δ
scripts/pipeline/orthology_benchmark.py
src/python/tests/test_orthology_benchmark.py
src/python/tests/test_utils.py
scripts/production/repair_mlss_tags.py
setup.py
src/python/tests/test_mlss_conf_parser.py
src/python/tests/test_filesys.py
src/python/tests/test_repair_mlss_tags.py
conftest.py
src/python/tests/test_hal_gene_liftover.py
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c7380d...4f20abd. Read the comment docs.

@CristiGuijarro CristiGuijarro self-requested a review July 29, 2022 07:11
Copy link
Contributor

@CristiGuijarro CristiGuijarro left a comment

Choose a reason for hiding this comment

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

For the purpose of this comment:

Not sure if these OSErrors should be tested at all. For some reason it rings a bell that the conclusion from one of the Dev Round Tables was no - because they originate from the system.

(And passing the tests) I have made a suggestion - this suggestion should mean you don't have to worry about what was discussed at the dev round table regarding testing OSErrors - and you just have to test that the message is printed and raises(CalledProcessError)? Not sure if this is much simpler, but it is something that was suggested to me at some point 🤔

Comment on lines +724 to +725
msg = f"Could not open a file '{output_file}' for writing."
raise OSError(msg) from e
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

Suggested change
msg = f"Could not open a file '{output_file}' for writing."
raise OSError(msg) from e
print(f"Could not open a file '{output_file}' for writing.")
sys.exit(1)

Instead of raising OSError after having excepted it? I gather that the purpose of excepting and then raising it is to print the msg for the user - I think this would achieve the same with less pain?

@dthybert dthybert requested a review from ens-sb August 19, 2022 09:54
twalsh-ebi pushed a commit to twalsh-ebi/ensembl-compara that referenced this pull request Aug 31, 2023
datacheck pipeline dependency module Search::Elasticsearh added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants