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

Add ONT FCs to bioinfo tab #446

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

aanil
Copy link
Member

@aanil aanil commented Nov 27, 2024

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 41 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@9ce7390). Learn more about missing BASE report.

Files with missing lines Patch % Lines
taca/utils/bioinfo_tab.py 0.00% 38 Missing ⚠️
taca/utils/cli.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #446   +/-   ##
=========================================
  Coverage          ?   27.83%           
=========================================
  Files             ?       37           
  Lines             ?     5489           
  Branches          ?        0           
=========================================
  Hits              ?     1528           
  Misses            ?     3961           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flowcell_info = (
couch_connection["nanopore_runs"].view("info/lims")[flowcell_id].rows[0]
)
if flowcell_info.value and "sample_data" in flowcell_info.value["loading"][0]:
Copy link
Member

Choose a reason for hiding this comment

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

I think I would like to see a case that catches the case where "loading" is not inside value and if it's an empty list. Even if it's "always" supposed to be there, I think it would be a shame if it broke the entire script run if it wasn't there.

Choose a reason for hiding this comment

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

In the case where the db .json has a "lims" nest, there should always be a subnest of "loading", and possibly "reloading". The db .json will not contain the "lims" nest until the LIMS step has been completed though.

Copy link
Member

Choose a reason for hiding this comment

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

Easy enough to add a check that the key exists here I think.

elif inst_brand == "ont":
base_name = os.path.basename(os.path.abspath(run_dir))
# Skip archived, no_backup, nosync and qc folders
if base_name in ["archived", "no_backup", "nosync", "qc"]:

Choose a reason for hiding this comment

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

Wouldn't base_name here be the name of the run, and not the dir in which it resides?

Choose a reason for hiding this comment

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

Oh, I see now. We are passing everything in the datadir into this function, including subdirs like nosyncand qc. I think it would be cleaner if we have some ealier logic to make sure only folders that look like ONT run dirs are passed into this function. The ONT run regex is available in the repo.

couch_connection["nanopore_runs"].view("info/lims")[flowcell_id].rows[0]
)
if flowcell_info.value and "sample_data" in flowcell_info.value["loading"][0]:
samples = flowcell_info.value["loading"][0]["sample_data"]

Choose a reason for hiding this comment

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

I think the index should be [-1], not [0]. If the LIMS script is re-run manually, we should use the latest values.

Copy link

@kedhammar kedhammar left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the potential downstream issues, but this seems like a good start for the integration. Nice work 👍

See comments for requested changes.

@aanil aanil requested a review from kedhammar December 6, 2024 09:23
@aanil aanil merged commit 70b77a8 into NationalGenomicsInfrastructure:master Dec 12, 2024
7 checks passed
aanil added a commit that referenced this pull request Dec 12, 2024
Forgot to commit fix in PR:  Add ONT FCs to bioinfo tab #446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants