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

ConvertJob.copy_sequences() method added. #145

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

charles-cowart
Copy link
Collaborator

Method allows caller to copy the fastq files associated w/a sample-name and a project into another project.
Needs unit-tests.

Method allows caller to copy the fastq files associated w/a sample-name
and a project into another project.
@charles-cowart charles-cowart requested a review from antgonza June 28, 2024 01:25
@charles-cowart
Copy link
Collaborator Author

Will add Amanda to the project as well.

@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9705977261

Details

  • 8 of 47 (17.02%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 79.914%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sequence_processing_pipeline/ConvertJob.py 8 47 17.02%
Totals Coverage Status
Change from base Build 9653458273: -1.5%
Covered Lines: 2071
Relevant Lines: 2401

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9719729459

Details

  • 8 of 61 (13.11%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.2%) to 79.236%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sequence_processing_pipeline/ConvertJob.py 8 61 13.11%
Totals Coverage Status
Change from base Build 9653458273: -2.2%
Covered Lines: 2071
Relevant Lines: 2415

💛 - Coveralls

@charles-cowart charles-cowart changed the title WIP: ConvertJob.copy_sequences() method added. ConvertJob.copy_sequences() method added. Jul 8, 2024
@coveralls
Copy link

coveralls commented Jul 8, 2024

Pull Request Test Coverage Report for Build 9912490051

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 128 of 131 (97.71%) changed or added relevant lines in 2 files are covered.
  • 16 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.9%) to 82.186%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sequence_processing_pipeline/ConvertJob.py 60 63 95.24%
Files with Coverage Reduction New Missed Lines %
sequence_processing_pipeline/tests/test_Pipeline.py 7 89.09%
sequence_processing_pipeline/Pipeline.py 9 84.52%
Totals Coverage Status
Change from base Build 9782509356: 0.9%
Covered Lines: 2187
Relevant Lines: 2480

💛 - Coveralls

@charles-cowart
Copy link
Collaborator Author

@antgonza Ready for review!

@charles-cowart
Copy link
Collaborator Author

charles-cowart commented Jul 9, 2024

Hi @AmandaBirmingham ! The functionality is now ready. Would you mind reviewing? Thank you!

Copy link
Collaborator

@AmandaBirmingham AmandaBirmingham left a comment

Choose a reason for hiding this comment

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

A few ideas, no major concerns. Only firmly requested change is updating usage in comments of the old name for the copy_all_replicates parameter.

sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/tests/test_ConvertJob.py Outdated Show resolved Hide resolved
Copy link
Contributor

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

Just a couple of things. I can re-review once @AmandaBirmingham changes are implemented.

sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
sequence_processing_pipeline/ConvertJob.py Outdated Show resolved Hide resolved
@charles-cowart
Copy link
Collaborator Author

Ready for review! I did a little bit of interpretation with @AmandaBirmingham 's last suggestion. Please let me know what you guys think.

copy_sequences() will now copy all replicates if a sample contains
replicates. Copying a single replicate is no longer an option.
@charles-cowart
Copy link
Collaborator Author

@AmandaBirmingham @antgonza ready for review!

@@ -146,6 +151,9 @@ def run(self, callback=None):
job_info = self.submit_job(self.job_script_path,
exec_from=self.log_path,
callback=callback)

self._get_sample_sheet_info()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, can this be called before the previous line or not (before submitting jobs)? Also, is the plan to unify this functionality in a single place? Maybe _get_sample_sheet_info()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function should only be called after the bcl-convert job returns completed. The output from this method is specific to supporting ConvertJob so there currently isn't a value in pushing it down into the base class. It's name does begin with '_' and so by convention it's an internal method and nobody should be calling it except ConvertJob.run().

@charles-cowart charles-cowart merged commit 22791b8 into biocore:master Nov 12, 2024
2 checks passed
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.

4 participants