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 sequencing artifact manager #407

Merged
merged 31 commits into from
Aug 3, 2023

Conversation

seallard
Copy link
Contributor

@seallard seallard commented Jul 27, 2023

This PR adds a class and utility functions for managing (retrieving and updating) artifacts from lims related to sequencing. This class will be used in the new script for the sample sequencing quality control. Part of project https://github.com/Clinical-Genomics/project-planning/issues/482.

So this class will be a component of the new script replacing https://github.com/Clinical-Genomics/clinical_EPPs/blob/master/EPPs/bcl2fastq.py.

I don't like how the new functions spread out across so many files, please let me know if there are other spots where they belong better or already exist. I don't think there was a super obvious structure to follow, so I bet that can be improved.

Added

  • Class for managing artifacts related to sequencing
  • Utility function for managing artifacts related to sequencing

This version is a:

  • MINOR - when you add functionality in a backwards compatible manner

Copy link

@ChrOertlin ChrOertlin left a comment

Choose a reason for hiding this comment

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

Hi looks nice. Made some comments about potential structure changes that could make it more robust with the use of BaseModels instead of nested dicts. Would likely require some work. But up to you.

I think @Karl-Svard has to approve changes here as the code owner, so commetning for now.

cg_lims/EPPs/files/sample_sheet/create_sample_sheet.py Outdated Show resolved Hide resolved
cg_lims/EPPs/qc/sequencing_artifact_manager.py Outdated Show resolved Hide resolved
cg_lims/get/artifacts.py Outdated Show resolved Hide resolved
cg_lims/get/artifacts.py Outdated Show resolved Hide resolved
cg_lims/get/artifacts.py Outdated Show resolved Hide resolved
cg_lims/get/fields.py Outdated Show resolved Hide resolved
cg_lims/set/udfs.py Outdated Show resolved Hide resolved
tests/EPPs/test_sequencing_artifact_manager.py Outdated Show resolved Hide resolved
@seallard seallard marked this pull request as draft July 28, 2023 11:23
@seallard seallard marked this pull request as ready for review July 28, 2023 12:53
@Karl-Svard
Copy link
Collaborator

Karl-Svard commented Aug 2, 2023

Nicely done! Just had some minor tweaks:

  • Renamed variables from sample_artifact to sample_lane_artifacts at several locations. This is to do with "sample artifacts" being an already established term in cg_lims. It's used for a particular type of artifact that all samples get when they originally get created.
  • Renamed get_artifact_lims_id to get_artifact_sample_id in order to better convey what it does.
  • Renamed some of the set functions to make it more clear that it doesn't necessarily change the values on the sample level.
  • Moved the test to match current folder structure.
  • Some linting with black.

Please take look and just ask if you wonder anything!

cg_lims/get/artifacts.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Karl-Svard Karl-Svard left a comment

Choose a reason for hiding this comment

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

Sweet! 💯

@Karl-Svard Karl-Svard merged commit 0fa45c7 into master Aug 3, 2023
2 checks passed
@Karl-Svard Karl-Svard deleted the 397-add-sequencing-artifact-manager branch August 3, 2023 08:31
@Karl-Svard Karl-Svard mentioned this pull request Aug 18, 2023
19 tasks
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