👔🔧 Add "organism" to pipeline config

[shnizzedy]

Fixes

Fixes #2175 by @shnizzedy
Related to #2225 by @shnizzedy
Fixes #2194 by @shnizzedy
Fixes 20572de by @e-kenneally
Fixes #1683 by @shnizzedy
Fixes #1952 by @e-kenneally
Fixes #1995 by @shnizzedy
Fixes #2245 by @shnizzedy

Description

• Adds pipeline_setup.organism = one of

  C-PAC/CPAC/pipeline/schema.py

  Line 80 in e798b43

  ORGANISMS: list[Organism] = ["human", "non-human primate", "rodent"]

• Deprecates calculate_motion_before and calculate_motion_after configuration options and refactors motion stack accordingly.

Technical details

• SSOTs

  C-PAC/CPAC/pipeline/engine.py

  Lines 537 to 539 in e798b43

  	def reg_tool(self, resource, strat=None) -> Optional[Literal["ants", "fsl"]]:
  	"""Check provenance for registration tool."""
  	return check_prov_for_regtool(self.get_cpac_provenance(resource, strat))

• Adds

  C-PAC/CPAC/func_preproc/func_motion.py

  Lines 479 to 486 in e798b43

  	def _pipe_suffix(estimate: bool, correct: bool, pipe_num: int) -> str:
  	"""Generate a suffix for the pipeline name based on estimate and correct flags."""
  	suffix = ""
  	if estimate:
  	suffix += "-estimate"
  	if correct:
  	suffix += "-correct"
  	return f"{suffix}_{pipe_num}"

  to clarify when we're estimating and when we're applying corrections (the same-name-different-pipe-number nodes were confusing me when I was debugging)
• Removes support for AFNI 3dECM < v21.1.1 (this was only used in variant images and added unnecessary complexity since we no longer build those).
• Updates some regressors and timeseries keys to desc-confounds_timeseries
• Creates

  C-PAC/CPAC/pipeline/schema.py

  Lines 348 to 358 in e798b43

  	class MotionEstimateFilter(TypedDict):
  	"""Type for motion estimate filter."""
  	filter_type: Literal["notch", "lowpass"]
  	filter_order: int
  	breathing_rate_min: OptionalType[float]
  	breathing_rate_max: OptionalType[float]
  	center_frequency: OptionalType[float]
  	filter_bandwidth: OptionalType[float]
  	lowpass_cutoff: OptionalType[float]
  	Name: OptionalType[str]

  for this type hint

  C-PAC/CPAC/func_preproc/func_motion.py

  Line 903 in e798b43

  opt: "MotionEstimateFilter",

• Updates some typehints and docstrings.
• Includes 🐛 Fix ideal_bandpass filter #2237 which fixes a bug that was causing benchmark-FLIRT to crash on the subject I was testing.
• Consolidates CPAC/registration/tests/test_registration_connectors.py.

Tests

• C-PAC/CPAC/nuisance/tests/test_bandpass.py

  Lines 98 to 137 in e798b43

  	@pytest.mark.skipif(
  	not getenv("OSF_DATA"),
  	reason="OSF API key not set in OSF_DATA environment variable",
  	)
  	def test_frequency_filter(tmp_path: Path) -> None:
  	"""Test that the bandpass filter works as expected."""
  	cfg = Preconfiguration("benchmark-FNIRT")
  	rpool = TestResourcePool(cfg)
  	wf = Workflow("bandpass_filtering", base_dir=str(tmp_path))
  	index = 0
  	for resource, file in {
  	"realigned_file": "residuals.nii.gz",
  	"regressor_file": "regressors.1D",
  	}.items():
  	rpool.osf(resource, file, tmp_path, index)
  	index += 1
  	filt = filtering_bold_and_regressors(
  	cfg["nuisance_corrections", "2-nuisance_regression", "Regressors"][0]
  	)
  	residuals = rpool.node_data("realigned_file")
  	regressors = rpool.node_data("regressor_file")
  	wf.connect(
  	[
  	(residuals.node, filt, [(residuals.out, "inputspec.functional_file_path")]),
  	(
  	regressors.node,
  	filt,
  	[(regressors.out, "inputspec.regressors_file_path")],
  	),
  	]
  	)
  	res: DiGraph = wf.run()
  	out_node = next(iter(res.nodes))
  	output = out_node.run()
  	trs = nib.load(output.outputs.bandpassed_file).header["dim"][4] # type: ignore[reportPrivateImportUsage]
  	array = load_censor_tsv(output.outputs.regressor_file, trs)
  	assert not all(
  	[array.min() == 0, array.max() == 0, array.sum() == 0]
  	), "Bandpass filter filtered all signals."

• C-PAC/CPAC/pipeline/test/test_schema_validation.py

  Lines 151 to 176 in e798b43

  	@pytest.mark.parametrize(
  	"configuration",
  	[
  	{},
  	{
  	"functional_preproc": {
  	"motion_estimates_and_correction": {
  	"motion_estimates": {
  	"calculate_motion_first": False,
  	"calculate_motion_after": False,
  	}
  	}
  	}
  	},
  	],
  	)
  	def test_deprecation(configuration: dict) -> None:
  	"""Test that deprecated options warn and non-deprecated options do not."""
  	if configuration:
  	with pytest.warns(DeprecationWarning) as record:
  	Configuration(configuration)
  	assert any("motion_estimates" in str(w.message) for w in record)
  	else:
  	with warnings.catch_warnings():
  	warnings.simplefilter("error")
  	Configuration(configuration)

• Updates

  C-PAC/CPAC/func_preproc/tests/test_preproc_connections.py

  Lines 105 to 261 in e798b43

  	@pytest.mark.parametrize(
  	",".join(_PARAMS.keys()), # run n=NUM_TESTS subset
  	sample(list(product(*_PARAMS.values())), NUM_TESTS),
  	)
  	def test_motion_filter_connections(
  	run: bool | list[bool],
  	filters: list[dict],
  	regtool: list[str],
  	calculate_motion_first: bool,
  	pre_resources: list[str],
  	motion_correction: list[list[str]],
  	) -> None:
  	"""Test that appropriate connections occur vis-à-vis motion filters."""
  	if isinstance(motion_correction, list) and len(motion_correction) != 1:
  	# Until https://github.com/FCP-INDI/C-PAC/issues/1935 is resolved
  	with pytest.raises(Invalid) as invalid:
  	c = Configuration(
  	{
  	"functional_preproc": {
  	"motion_estimates_and_correction": {
  	"motion_correction": {"using": motion_correction}
  	}
  	}
  	}
  	)
  	assert "FCP-INDI/C-PAC/issues/1935" in invalid
  	return
  	# parameterized Configuration
  	c = Configuration(
  	{
  	"functional_preproc": {
  	"motion_estimates_and_correction": {
  	"motion_correction": {"using": motion_correction},
  	"motion_estimate_filter": {"run": run, "filters": filters},
  	"run": True,
  	},
  	"run": True,
  	},
  	"nuisance_corrections": {
  	"2-nuisance_regression": {
  	"Regressors": [
  	{
  	"Name": "aCompCor, GSR, no censor",
  	"Motion": {
  	"include_delayed": True,
  	"include_squared": True,
  	"include_delayed_squared": True,
  	},
  	"aCompCor": {
  	"summary": {"method": "DetrendPC", "components": 5},
  	"tissues": ["WhiteMatter", "CerebrospinalFluid"],
  	"extraction_resolution": 3,
  	},
  	"GlobalSignal": {"summary": "Mean"},
  	"PolyOrt": {"degree": 2},
  	"Bandpass": {
  	"bottom_frequency": 0.01,
  	"top_frequency": 0.1,
  	},
  	}
  	]
  	}
  	},
  	}
  	)
  	# resource for intial inputs
  	before_this_test = create_dummy_node("created_before_this_test", pre_resources)
  	rpool = ResourcePool(cfg=c)
  	for resource in pre_resources:
  	if resource.endswith("xfm"):
  	node_name = f"created_before_this_test_{regtool}"
  	elif resource == "desc-movementParameters_motion":
  	node_name = f"created_before_this_test_{motion_correction}"
  	else:
  	node_name = "created_before_this_test"
  	rpool.set_data(resource, before_this_test, resource, {}, "", node_name)
  	# set up blocks
  	pipeline_blocks = []
  	func_blocks = {key: [] for key in ["init", "preproc", "mask"]}
  	func_blocks["prep"] = [
  	func_normalize,
  	[
  	coregistration_prep_vol,
  	coregistration_prep_mean,
  	coregistration_prep_fmriprep,
  	],
  	]
  	pipeline_blocks += stack_motion_blocks(func_blocks, c, rpool)
  	# Nuisance Correction
  	generate_only = (
  	True not in c["nuisance_corrections", "2-nuisance_regression", "run"]
  	)
  	if not rpool.check_rpool("desc-cleaned_bold"):
  	pipeline_blocks += choose_nuisance_blocks(c, generate_only)
  	wf = Workflow(re.sub(r"[\[\]\-\:\_ \'\",]", "", str(rpool)))
  	connect_pipeline(wf, c, rpool, pipeline_blocks)
  	# Check that filtering is happening as expected
  	filter_switch_key = [
  	"functional_preproc",
  	"motion_estimates_and_correction",
  	"motion_estimate_filter",
  	"run",
  	]
  	if c.switch_is_on(filter_switch_key, exclusive=True):
  	assert all(
  	strat.filtered_movement
  	for strat in rpool.get_strats(["desc-movementParameters_motion"]).values()
  	)
  	elif c.switch_is_off(filter_switch_key, exclusive=True):
  	assert not any(
  	strat.filtered_movement
  	for strat in rpool.get_strats(["desc-movementParameters_motion"]).values()
  	)
  	elif c.switch_is_on_off(filter_switch_key):
  	assert any(
  	strat.filtered_movement
  	for strat in rpool.get_strats(["desc-movementParameters_motion"]).values()
  	)
  	if (
  	"mcflirt"
  	in c[
  	"functional_preproc",
  	"motion_estimates_and_correction",
  	"motion_correction",
  	"using",
  	]
  	and "desc-movementParameters_motion" not in pre_resources
  	):
  	# Only for [On, Off] + mcflirt, we should have at least one of each
  	assert {
  	wf.get_node(nodename).inputs.calc_from
  	for nodename in wf.list_node_names()
  	if nodename.endswith(".calculate_FDJ")
  	and wf.get_node(nodename).inputs.calc_from is not Undefined
  	} == {"affine", "rms"}
  	regressor_subwfs = [
  	wf.get_node(nodename[:-26])
  	for nodename in wf.list_node_names()
  	if nodename.endswith("build_nuisance_regressors")
  	]
  	for subwf in regressor_subwfs:
  	# a motion filter is an input to the nuisance regressor subworkflow
  	is_filtered = []
  	# a motion filter should be an input to the regressor subworkflow
  	should_be_filtered = "_filt-" in subwf.name and "_filt-none" not in subwf.name
  	for u, v in wf._graph.edges: # pylint: disable=invalid-name,protected-access
  	if (
  	v == subwf
  	and hasattr(u, "interface")
  	and isinstance(u.interface, (NipypeFunction, CpacFunction))
  	and "notch_filter_motion" in u.interface.inputs.function_str
  	):
  	is_filtered.append(u)
  	assert bool(is_filtered) == should_be_filtered, _filter_assertion_message(
  	subwf, is_filtered, should_be_filtered
  	)

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the develop branch of the repository.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I updated the changelog.
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[codecov]

Codecov Report

Attention: Patch coverage is 62.55708% with 164 lines in your changes missing coverage. Please review.

Project coverage is 28.7%. Comparing base (a2c3d2f) to head (2afdf21).
Report is 52 commits behind head on develop.

Files with missing lines	Patch %	Lines
CPAC/func_preproc/func_motion.py	62.8%	51 Missing and 10 partials ⚠️
CPAC/registration/registration.py	34.0%	32 Missing and 3 partials ⚠️
CPAC/pipeline/engine.py	28.6%	14 Missing and 1 partial ⚠️
CPAC/pipeline/cpac_pipeline.py	8.3%	10 Missing and 1 partial ⚠️
...PAC/func_preproc/tests/test_preproc_connections.py	0.0%	9 Missing ⚠️
CPAC/nuisance/nuisance.py	61.5%	5 Missing ⚠️
CPAC/utils/monitoring/monitoring.py	0.0%	4 Missing and 1 partial ⚠️
CPAC/utils/tests/osf.py	70.6%	3 Missing and 2 partials ⚠️
CPAC/qc/xcp.py	0.0%	4 Missing ⚠️
CPAC/pipeline/schema.py	92.3%	1 Missing and 2 partials ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #2230     +/-   ##
=========================================
+ Coverage     28.4%   28.7%   +0.3%     
=========================================
  Files          230     230             
  Lines        26052   26169    +117     
  Branches      4092    4113     +21     
=========================================
+ Hits          7397    7503    +106     
+ Misses       18024   18018      -6     
- Partials       631     648     +17     

Files with missing lines	Coverage Δ
CPAC/_entrypoints/run.py	9.0% <ø> (+0.1%)	⬆️
CPAC/func_preproc/__init__.py	100.0% <ø> (ø)
CPAC/network_centrality/network_centrality.py	88.7% <100.0%> (+4.5%)	⬆️
CPAC/nuisance/bandpass.py	42.7% <ø> (ø)
CPAC/nuisance/tests/test_bandpass.py	100.0% <100.0%> (ø)
CPAC/pipeline/nipype_pipeline_engine/engine.py	30.8% <100.0%> (ø)
CPAC/pipeline/nodeblock.py	97.4% <100.0%> (+0.3%)	⬆️
CPAC/pipeline/test/test_schema_validation.py	100.0% <100.0%> (ø)
CPAC/qc/pipeline.py	25.0% <ø> (ø)
...registration/tests/test_registration_connectors.py	100.0% <100.0%> (ø)
... and 21 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sgiavasis]

Me to @shnizzedy :
Bro the organism PR is a colossus 😂 I feel like I started reviewing it in the 4th grade

@shnizzedy :
🤣

Me:
"Oh, I'm almost done"
[4 more Load diffs]
"Oh no." 💀

Great work, approved ✅