Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions good_standard_metagv0.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
[Header],,,,,,,,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This file was not previously in source control. Where does it come from and why do you feel it should be added?

IEMFileVersion,4,,,,,,,
SheetType,tellseq_metag,,,,,,,
SheetVersion,10,,,,,,,
Investigator Name,Knight,,,,,,,
Experiment Name,RKL_experiment,,,,,,,
Date,2025-08-14,,,,,,,
Workflow,GenerateFASTQ,,,,,,,
Application,FASTQ Only,,,,,,,
Assay,Metagenomic,,,,,,,
Description,,,,,,,,
Chemistry,Default,,,,,,,
,,,,,,,,
[Reads],,,,,,,,
151,,,,,,,,
151,,,,,,,,
,,,,,,,,
[Settings],,,,,,,,
ReverseComplement,0,,,,,,,
,,,,,,,,
[Data],,,,,,,,
Sample_ID,Sample_Name,Sample_Plate,well_id_384,barcode_id,Sample_Project,Well_description,Lane,
sample_1,sample.1,A1,False,C501,MyProject_99999,A1.sample.1.False,1,
sample_2,sample.2,A2,False,C509,MyProject_99999,A2.sample.2.False,1,
sample_3,sample.3,A3,False,C520,MyProject_99999,A3.sample.3.False,1,
,,,,,,,,
[Bioinformatics],,,,,,,,
Sample_Project,QiitaID,BarcodesAreRC,ForwardAdapter,ReverseAdapter,HumanFiltering,library_construction_protocol,experiment_design_description,contains_replicates
MyProject_99999,99999,True,AACC,GGTT,False,some protocol,some description,False
,,,,,,,,
[Contact],,,,,,,,
Sample_Project,Email,,,,,,,
MyProject_99999,foo@bar.org,,,,,,,
,,,,,,,,
[SampleContext],,,,,,,,
sample_name,sample_type,primary_qiita_study,secondary_qiita_studies,,,,,
sample.3,control blank,99999,,,,,,
,,,,,,,,
1,466 changes: 1,387 additions & 79 deletions notebooks/tellseq_C_equal_volume_pooling.ipynb

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions notebooks/tellseq_D_variable_volume_pooling.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@
"outputs": [],
"source": [
"## INPUT\n",
"# when paramaters does not exist use the variable value, and if exhists, use the key in the test dic.\n",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Typos in the comment :)

"paramaters" -> "parameters"
"does" -> "do
"exhists" -> "exist"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fix typos in the comment.

"iseqnormed_picklist_fbase = assign_input(variable_value='./test_output/Pooling/Tellseq_iSeqnormpool',\n",
" test_dict=test_dict,\n",
" key='iseqnormed_picklist_fbase')"
Expand Down
24 changes: 12 additions & 12 deletions notebooks/test_output/QC/Tellseq_plate_df_C_set_col19to24.txt

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Experiment Name,RKLtest_col19to24,,,,,,,,,,
Investigator Name,Enter the investigator name (optional),,,,,,,,,,
Project Name,TestProjA_10002_1_2_3_4_10to1dilution,,,,,,,,,,
Date,2025-04-01,,,,,,,,,,
Date,2025-09-04,,,,,,,,,,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change, while little, is an indication of a drawback of the current testing method. Some of the files that are produced by the notebook (the kind called "samplesheet" files) include the date on which they were produced--it will be different every day that they are run! This means that our method of just checking line-by-line for perfect matches will always fail with this kind of file. It wasn't an issue for notebook D, because notebook D doesn't produce that kind of output file ... but notebook C produces three of them! This suggests that the testing approach used by notebook D is not robust enough to work for all the other notebooks.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, thanks for pointing out this!
I have revised the test logic to verify the output file names and their locations, rather than performing a line-by-line content comparison.

Workflow,GenerateFASTQ,,,,,,,,,,
Library Prep Kit,TELLSEQ,,,,,,,,,,
[Manifest],,,,,,,,,,,
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

159 changes: 159 additions & 0 deletions notebooks/tests/test2_tellseq_C_equal_volume_pooling.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import unittest

@AmandaBirmingham AmandaBirmingham Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see that there are TWO tellseq C test files in this commit, and neither of them has the precise name that I would expect (test_tellseq_C_equal_volume_pooling.py). I am guessing that this one is the one you want to use and that the other one is an earlier version that has been superseded, but please let me know if I am right about this.

import papermill as pm
import tempfile
from pathlib import Path
import os
import shutil

NOTEBOOK = "tellseq_C_equal_volume_pooling.ipynb"

# Golden files live under notebooks/test_output/<subdir>/<filename>
EXPECTED_SUBDIRS = ["Pooling", "Indices", "SampleSheets", "QC"]

# Only compare data files (skip .ipynb, images, etc.)
DATA_EXTS = {".txt", ".csv", ".tsv"}


class TestTellseqC_ShadowWorkspace(unittest.TestCase):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What does the _ShadowWorkspace suffix here indicate?

def setUp(self):
# project root (parent of this test file’s folder)
self.notebooks_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
self.repo_test_output_dir = os.path.join(self.notebooks_dir, "test_output")

# sanity: required inputs exist in repo
must_inputs = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is very careful programming, and I can see that you are looking out for error cases. However, since this is NOT a user-facing tool (these unit tests will be run by the code developers only), it is not necessary to employ this level of caution (and complication). We can take the python approach of Easier to Ask Forgiveness than Permission (EAFP, see https://realpython.com/ref/glossary/eafp/ ) and just let the code fail if it tries to access the input files and they aren't there. The unit test framework already has error-catching baked in, and the built-in python error thrown in the case of a missing file will be informative enough for a programmer to figure out what is going on :)

os.path.join(self.repo_test_output_dir, "QC", "Tellseq_plate_df_B.txt"),
os.path.join(self.repo_test_output_dir, "QC", "Tellseq_expt_info.yml"),
]
for fp in must_inputs:
if not os.path.isfile(fp):
raise FileNotFoundError(f"[SETUP] Required input not found in repo: {fp}")

def _find_expected_path(self, filename: str) -> str:
"""Find golden file by filename under test_output/<subdir>/*."""
for sub in EXPECTED_SUBDIRS:
candidate = os.path.join(self.repo_test_output_dir, sub, filename)
if os.path.exists(candidate):
return candidate
raise AssertionError(
f"[COMPARE] Expected golden file not found for '{filename}'. "
f"Searched under: {', '.join(EXPECTED_SUBDIRS)}"
)

def _assert_text_files_equal(self, produced_fp: Path, expected_fp: Path):
"""Strict line-by-line text comparison with helpful diffs."""
with open(produced_fp, "r") as f_out, open(expected_fp, "r") as f_exp:
out_lines = f_out.readlines()
exp_lines = f_exp.readlines()

self.assertEqual(
len(out_lines), len(exp_lines),
msg=(f"[DIFF] Line count differs for '{produced_fp.name}': "
f"produced={len(out_lines)} vs expected={len(exp_lines)}")
)

for i, (o, e) in enumerate(zip(out_lines, exp_lines), start=1):
if o != e:
self.fail(
f"[DIFF] Mismatch in '{produced_fp.name}' at line {i}.\n"
f"Produced: {o.rstrip()}\n"
f"Expected: {e.rstrip()}\n"
)

def test_equal_volume_pooling_outputs_shadow(self):
"""
Keep Notebook C's hardcoded './test_output/...' as-is by creating that
structure under a temp dir and running with cwd=tmp. Then compare tmp outputs
vs repo goldens by filename.
"""
with tempfile.TemporaryDirectory() as tmp_dir:
tmp_root = Path(tmp_dir) # shadow root = './'
tmp_out = tmp_root / "test_output" # ./test_output
tmp_qc = tmp_out / "QC"
tmp_idx = tmp_out / "Indices"
tmp_ss = tmp_out / "SampleSheets"

# create expected dirs under tmp
for d in (tmp_qc, tmp_idx, tmp_ss):
d.mkdir(parents=True, exist_ok=True)

# copy required inputs from repo -> tmp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is no need to do this--you can use the inputs straight from their current location!

shutil.copy2(
os.path.join(self.repo_test_output_dir, "QC", "Tellseq_plate_df_B.txt"),
tmp_qc / "Tellseq_plate_df_B.txt",
)
shutil.copy2(
os.path.join(self.repo_test_output_dir, "QC", "Tellseq_expt_info.yml"),
tmp_qc / "Tellseq_expt_info.yml",
)


run_params = {

@AmandaBirmingham AmandaBirmingham Oct 31, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Although you have provided run parameters here and are passing them into the notebook in the execute_notebook command, these parameters will never be used. The reason is all the lines in notebook C that set these parameters--as the first example, the cell that reads

## INPUT
current_set_id = "col19to24"

Your code will set all parameters in the top cell, but then they will be OVERWRITTEN by the individual statements throughout the code that currently set inputs. As we discussed during our meeting, in order for this test approach to work, it will be necessary to comment out the code lines in notebook C that set the inputs. This will be fine--the users can comment them back in when they go to run the code (at which point they will need to enter their own particular input values anyway).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the detailed feedback! You're absolutely right. I realized that if those parameters are redefined in subsequent cells, the values injected by Papermill would be overwritten.
As we discussed, I have commented out the hardcoded input lines throughout Notebook C and tagged the top input cell as a 'parameters' cell. This ensures Papermill correctly overrides the default values during the test execution.

"current_set_id": "col19to24",
"total_vol": 190,
"iseq_sequencer": "iSeq",
"novaseq_sequencer": "NovaSeqXPlus",
"full_plate_fp": "./test_output/QC/Tellseq_plate_df_B.txt",
"expt_config_fp": "./test_output/QC/Tellseq_expt_info.yml",
}

# run notebook with cwd switched to tmp_root so './test_output' points to tmp
prev_cwd = os.getcwd()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do not think this is the direction we want to go ... changing the working directory is a lot more work (and more error prone) than simply changing the input path strings!

try:
os.chdir(tmp_root)
pm.execute_notebook(
input_path=os.path.join(self.notebooks_dir, NOTEBOOK),
output_path=str(tmp_root / "executed_test_equal_volume_pooling.ipynb"),
parameters=run_params,
log_output=True,
)
finally:
os.chdir(prev_cwd)

# collect produced data files under tmp/test_output (skip .ipynb)
produced_files = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is an interesting elaboration that I hadn't thought of--checking if the code emitted any unexpected files as well as checking that it did create the expected ones. If we were going this direction, I think it would not make sense to look only for unexpected .txt/.tsv/.csv files, though. I think instead one would want to exclude all expected files (including the ipynb) and all hidden files and then report anything ELSE, regardless of extension!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh wow! Thanks for pointing that out! I have updated the test to be much more strict. Instead of just looking for specific data extensions, I now:
1.Whitelisted all expected files (including the executed .ipynb).
2.Filtered out hidden system files (starting with .).
3.Flagged anything ELSE as a failure.

This ensures the notebook workspace stays perfectly clean and doesn't produce any unexpected side-effect files.

p for p in tmp_root.glob("test_output/**/*")
if p.is_file() and p.suffix.lower() in DATA_EXTS
]
self.assertTrue(
produced_files,
msg="[CHECK] No data outputs (.txt/.csv/.tsv) were produced under tmp/test_output."
)

# --- Expect exactly the names you observed were produced ---
expected_names = {
"Tellseq_evp_set_col19to24.txt",
"Tellseq_plate_df_B.txt",
"Tellseq_plate_df_C_set_col19to24.txt",
"Tellseq_samplesheet_instrument_iseq_set_col19to24.csv",
"Tellseq_samplesheet_spp_iseq_set_col19to24.csv",
"Tellseq_samplesheet_spp_novaseqxplus_set_col19to24.csv",
}

# helpful once: see what was actually produced
print("Produced files:", sorted(p.name for p in produced_files))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This sort of statement is useful while debugging but would want to remove it for production code (even production unit tests :D )

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oh okay! i will!! Thanks for letting me know!


produced_names = {p.name for p in produced_files}
missing = expected_names - produced_names
extra = produced_names - expected_names
self.assertFalse(missing, f"Missing outputs: {sorted(missing)}")
self.assertFalse(extra, f"Unexpected outputs: {sorted(extra)}")

# compare each produced file to its golden
for name in sorted(expected_names):
produced_fp = next(p for p in produced_files if p.name == name)
expected_fp = self._find_expected_path(name)

# guard: prevent self-compare if paths somehow coincide
if Path(expected_fp).resolve() == produced_fp.resolve():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting ... I think it would never have occurred to me to test for this :) While I still think it is easier to just pass in the directories explicitly, I congratulate you on identifying an unusual edge case in the code ecosystem you have set up!

self.fail(
"[GUARD] Produced file path equals expected golden path:\n"
f" {produced_fp}\n"
"Ensure notebook wrote into tmp and goldens remain in repo."
)

self._assert_text_files_equal(produced_fp, Path(expected_fp))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note that currently the test will ALWAYS fail due to the date situation in the samplesheet files

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, thanks for pointing that out! I've updated the logic for Test C. I revised the test to verify the output file names and locations instead of performing a line-by-line content comparison, making it more robust against dynamic data like dates.



if __name__ == "__main__":
unittest.main()
70 changes: 70 additions & 0 deletions notebooks/tests/test_tellseq_C_equal_volume_pooling_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import unittest
import papermill as pm
import tempfile
from pathlib import Path
import os

NOTEBOOK = "tellseq_C_equal_volume_pooling.ipynb"
PICKLIST_FNAME = "Tellseq_iSeqnormpool_set_col19to24.txt"


class TestTellseqD(unittest.TestCase):
def setUp(self):
self.notebooks_dir = os.path.dirname(os.path.dirname(__file__))
self.test_output_dir = os.path.join(self.notebooks_dir, 'test_output')

def test_iseqnorm_picklist(self):
"""Verify notebook produces expected output for iSeqnormed picklist."""

with tempfile.TemporaryDirectory() as tmp_dir:
tmp_path = Path(tmp_dir)

run_params = {
"""
'plate_df_set_fp': f"{self.test_output_dir}/QC/"
f"Tellseq_plate_df_C_set_col19to24.txt",
'read_counts_fps': [
f"{self.notebooks_dir}/test_data/Demux/"
f"Tellseq_fastqc_sequence_counts.tsv"],
'dynamic_range': 5,
'iseqnormed_picklist_fbase':
f"{tmp_path}/Tellseq_iSeqnormpool"
"""
'full_plate_fp' : f"{self.test_output_dir}/QC/Tellseq_plate_df_B.txt",
'expt_config_fp' : f"{self.test_output_dir}/QC/Tellseq_expt_info.yml",
'current_set_id' : "col19to24",
'total_vol' : 190,
'evp_picklist_fbase' : f"{self.test_output_dir}/Indices/Tellseq_evp",
'machine_samplesheet_fbase' : f"{self.test_output_dir}/SampleSheets/Tellseq_samplesheet_instrument_iseq",
'spp_samplesheet_fbase' : f"{self.test_output_dir}/SampleSheets/Tellseq_samplesheet_spp",
'iseq_sequencer' : 'iSeq',
'novaseq_sequencer' : 'NovaSeqXPlus'

}

pm.execute_notebook(
input_path=f"{self.notebooks_dir}/{NOTEBOOK}",
output_path=f"{tmp_path}/test_iseqnorm_picklist.ipynb",
parameters=run_params,
log_output=True,
)

out_iseqnormed_picklist_fp = f"{tmp_path}/{PICKLIST_FNAME}"
self.assertTrue(os.path.exists(out_iseqnormed_picklist_fp),
msg="Notebook did not produce desired file.")

exp_iseqnormed_fp = \
f"{self.test_output_dir}/Pooling/{PICKLIST_FNAME}"
with open(out_iseqnormed_picklist_fp, 'r') as out:
with open(exp_iseqnormed_fp, 'r') as test:
out_lines = out.readlines()
test_lines = test.readlines()
for out_line, test_line in zip(out_lines,
test_lines):
self.assertEqual(out_line, test_line,
msg=("Lines of output" +
"and test don't match"))


if __name__ == "__main__":
unittest.main()
31 changes: 20 additions & 11 deletions notebooks/tests/test_tellseq_D_variable_volume_pooling.py
Original file line number Diff line number Diff line change
@@ -1,26 +1,36 @@
'''
1.Checking whether the expected output file (Tellseq_iSeqnormpool_set_col19to24.txt) was created.
2.Comparing the contents of that file to a known "expected" version of the file.
'''

# imports are executed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment is not useful since a programmer reading the code will know this already from seeing the import statements. The same is true for many of the comments below ("define constants variable", "class definition is executed", etc). If you add comments like this to help yourself as you are becoming familiar with new code, you will want to take them out before committing to repository.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have removed all the comments.

import unittest
import papermill as pm
import tempfile
from pathlib import Path
import os

# define constants variable
NOTEBOOK = "tellseq_D_variable_volume_pooling.ipynb"
PICKLIST_FNAME = "Tellseq_iSeqnormpool_set_col19to24.txt"


# class definition is executed
class TestTellseqD(unittest.TestCase):
#method 1. setup

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For production python code, you will want to provide function comments in docstrings (see https://peps.python.org/pep-0257/#what-is-a-docstring ). There are a number of different formats for docstrings (see https://stackoverflow.com/a/24385103 ) and I am sorry to say that our projects have not selected a single one and stuck to it ... mostly people in the lab use either the reST or the Numpydoc formats.

# automatically before every test_... method-
def setUp(self):
self.notebooks_dir = os.path.dirname(os.path.dirname(__file__))
self.test_output_dir = os.path.join(self.notebooks_dir, 'test_output')
self.notebooks_dir = os.path.dirname(os.path.dirname(__file__)) # /Users/seosonghee/Documents/KnightLab/kl-metapool/notebooks

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please delete any info like this that is specific to your own machine.

self.test_output_dir = os.path.join(self.notebooks_dir, 'test_output') # the "<project_root>/test_output"

def test_iseqnorm_picklist(self):
"""Verify notebook produces expected output for iSeqnormed picklist."""
#method 2. test_iseqnorm_picklist (Starts with test_)
# creates the expected output file,
# matches an expected “golden” file line-by-line.

def test_iseqnorm_picklist(self):
with tempfile.TemporaryDirectory() as tmp_dir:
tmp_path = Path(tmp_dir)

# Build the parameters to inject into the notebook
run_params = {
'test_dict': {
'plate_df_set_fp': f"{self.test_output_dir}/QC/"
f"Tellseq_plate_df_C_set_col19to24.txt",
'read_counts_fps': [
Expand All @@ -29,16 +39,15 @@ def test_iseqnorm_picklist(self):
'dynamic_range': 5,
'iseqnormed_picklist_fbase':
f"{tmp_path}/Tellseq_iSeqnormpool"
}
}

# run the notebook /test

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment is misleading: this code runs the notebook , but not the test.

pm.execute_notebook(
input_path=f"{self.notebooks_dir}/{NOTEBOOK}",
output_path=f"{tmp_path}/test_iseqnorm_picklist.ipynb",
parameters=run_params,
log_output=True,
)

# If the notebook didn’t write Tellseq_iSeqnormpool_set_col19to24.txt, the test fails here.
out_iseqnormed_picklist_fp = f"{tmp_path}/{PICKLIST_FNAME}"
self.assertTrue(os.path.exists(out_iseqnormed_picklist_fp),
msg="Notebook did not produce desired file.")
Expand All @@ -55,6 +64,6 @@ def test_iseqnorm_picklist(self):
msg=("Lines of output" +
"and test don't match"))


# run the file directly
if __name__ == "__main__":
unittest.main()
Loading