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

Feature/fixParseCNV #160

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Feature/fixParseCNV #160

wants to merge 10 commits into from

Conversation

melferink
Copy link
Member

  • fixed bug in counting samples numbers due to multiple lanes

Copy link

@fdekievit fdekievit left a comment

Choose a reason for hiding this comment

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

Please see comments

ParseCNVQC/parse_cnv_qc.py Outdated Show resolved Hide resolved
ParseCNVQC/tests/test_parse_cnv_qc.py Outdated Show resolved Hide resolved
ParseCNVQC/tests/run3/SampleSheet.csv Outdated Show resolved Hide resolved
ParseCNVQC/tests/run1/SampleSheet.csv Outdated Show resolved Hide resolved
@@ -62,6 +62,51 @@ def make_mail(today, daysago, attachment, run_status):
send_email(settings.email_from, settings.email_to, subject, text, attachment)


def get_number_samples_per_run_from_samplesheet(folder, rawfolder, projects, warnings):

Choose a reason for hiding this comment

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

ik zou hier de argumenten wat meer specifieker maken, b.v. run_folder ipv folder

number_samples_run = 0
lanes = []
lane_index = ""
if os.path.exists("{}/SampleSheet.csv".format(folder)):

Choose a reason for hiding this comment

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

ik zie dat we vaan verschillende soorten python strings door elkaar gebruiken.
wat er nu staat is niet fout ofzo, maar zouden we misschien voor de nieuwere f-strings kunnen kiezen?

Dan zou deze zin:
if os.path.exists(f"{folder}/SampleSheet.csv"):
worden (en de rest van de strings worden dan ook net anders, maar dat scheelt weer text en het is meestal leesbaarder)

number_samples_run += 1
if line.split(",")[lane_index] not in lanes:
lanes.append(line.split(",")[lane_index])
if "Sample_ID" not in line:

Choose a reason for hiding this comment

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

sorry maar de logica klopt, alleen ik vind het niet heel leesbaar.

als ik het goed begrijp lezen we dus elke lijn van een samplesheet, en als we niet Sample_ID in de regel vinden gaan we door, maar als we in de else terecht komen gaan we een ander codeblock in waarin we dan in sample_section zitten.

ik denk dat een config parser hier de oplossing is:
https://docs.python.org/3/library/configparser.html
dan kan je een config section lezen uit een samplesheet a la
import configparser
config = configparser.ConfigParser()
samplesheet = config.read(samplesheet_path)

je hebt dan 1 sample block met alle samples, en die kan je makkelijk filteren met een for-loopje ofzo.

is een idee, wellicht werkt t niet, maar t proberen waard :)

# prevent division by zero.
if len(lanes) > 0:
rawfolder[folder][1] += number_samples_run/len(lanes)
return rawfolder, warnings

Choose a reason for hiding this comment

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

mocht er tijd voor zijn, hou dit stukje dan eens kritisch tegen het licht.

op dit moment zit er namelijk logica in de functie om het aantal samples in een samplesheet te bepalen, maar het doet ook dingen daar buitenom, b.v. een dict bijhouden met het aantal samples per project. eigenlijk mag dat best zijn eigen functie zijn.

en een mooi voorbeeldje van code die heerlijk zou werken met OO!
dan kan je gewoon een run object maken dat 1 of meerdere projects heeft en de determine_samples uitvoeren op een run object, die het op alle (gefilterde) project lijst uitvoert

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