From a211fd1cb64be86077c89c594bde0ec7c245f787 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 17:31:35 -0500 Subject: [PATCH 1/5] drop participant rows that don't have BIDS data --- src/brainflex/core/get_data.py | 14 +++++++------ tests/core/test_get_data.py | 36 ++++++++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/brainflex/core/get_data.py b/src/brainflex/core/get_data.py index 5f44f45..f33f035 100644 --- a/src/brainflex/core/get_data.py +++ b/src/brainflex/core/get_data.py @@ -181,9 +181,9 @@ def _get_freesurfer( def _write_pheno(layout: DatasetLayout, session_ids: dict[str, str]) -> None: """Copy participants.tsv to the tabular directory, with a session ID column. - Participants without BIDS imaging data get an empty session ID. Warns if - participant IDs and BIDS subjects do not fully match, and raises ValueError - if none match. + Rows with an empty participant ID and participants without BIDS imaging + data are excluded. Warns if participant IDs and BIDS subjects do not fully match, + and raises ValueError if none match. Also copies participants.json, if available. """ @@ -194,7 +194,7 @@ def _write_pheno(layout: DatasetLayout, session_ids: dict[str, str]) -> None: with fpath_participants.open(newline="") as file: reader = csv.DictReader(file, delimiter="\t") fieldnames = reader.fieldnames - rows = list(reader) + rows = [row for row in reader if (row[COL_PARTICIPANT_ID] or "").strip()] fieldnames = [COL_PARTICIPANT_ID, COL_SESSION_ID] + [ col for col in fieldnames if col not in (COL_PARTICIPANT_ID, COL_SESSION_ID) @@ -207,7 +207,8 @@ def _write_pheno(layout: DatasetLayout, session_ids: dict[str, str]) -> None: ) if missing := sorted(participant_ids - session_ids.keys()): warnings.warn( - f"participants in {PARTICIPANTS_FILENAME} without BIDS data: {missing}", + f"Found participants in {PARTICIPANTS_FILENAME} without BIDS data: " + f"{missing}. Excluding them from {PHENO_FILENAME}.", stacklevel=3, ) if missing := sorted(session_ids.keys() - participant_ids): @@ -216,8 +217,9 @@ def _write_pheno(layout: DatasetLayout, session_ids: dict[str, str]) -> None: stacklevel=3, ) + rows = [row for row in rows if row[COL_PARTICIPANT_ID] in session_ids] for row in rows: - row[COL_SESSION_ID] = session_ids.get(row[COL_PARTICIPANT_ID], "") + row[COL_SESSION_ID] = session_ids[row[COL_PARTICIPANT_ID]] layout.dpath_tabular.mkdir(parents=True, exist_ok=True) with (layout.dpath_tabular / PHENO_FILENAME).open("w", newline="") as file: diff --git a/tests/core/test_get_data.py b/tests/core/test_get_data.py index 341b8ea..97d7c5b 100644 --- a/tests/core/test_get_data.py +++ b/tests/core/test_get_data.py @@ -26,7 +26,7 @@ def _raw_objects(subject_dirs: list[str]) -> dict[tuple[str, str], bytes]: (OPENNEURO_BUCKET, f"{DATASET_ID}/dataset_description.json"): b"{}", (OPENNEURO_BUCKET, f"{DATASET_ID}/participants.json"): b'{"age": {}}', (OPENNEURO_BUCKET, f"{DATASET_ID}/participants.tsv"): ( - b"participant_id\tage\nsub-01\t20\nsub-02\t30\nsub-03\t40\n" + b"participant_id\tage\nsub-01\t20\nsub-02\t30\n" ), (OPENNEURO_BUCKET, f"{DATASET_ID}/derivatives/file.txt"): b"skipped", (OPENNEURO_BUCKET, f"{DATASET_ID}/sub-01/"): b"", @@ -95,8 +95,7 @@ def root(tmp_path: Path) -> Path: return tmp_path -# the mock dataset has a participant without BIDS data -@pytest.mark.filterwarnings("ignore:participants in participants.tsv without BIDS data") +@pytest.mark.filterwarnings("error") class TestGetData: def test_get_data(self, mock_s3: dict[tuple[str, str], bytes], root: Path): dpath_study = get_data(root, DATASET_ID) @@ -122,12 +121,10 @@ def test_get_data(self, mock_s3: dict[tuple[str, str], bytes], root: Path): assert not (dpath_output / "sub-01/mri").exists() pheno = (dpath_study / "tabular" / "pheno.tsv").read_text() - # sub-03 has no imaging data assert pheno == ( "participant_id\tsession_id\tage\n" "sub-01\tses-unnamed\t20\n" "sub-02\tses-unnamed\t30\n" - "sub-03\t\t40\n" ) pheno_json = (dpath_study / "tabular" / "pheno.json").read_text() assert pheno_json == '{"age": {}}' @@ -162,8 +159,32 @@ def test_participants_without_bids( mock_s3: dict[tuple[str, str], bytes], root: Path, ): - with pytest.warns(UserWarning, match=r"without BIDS data: \['sub-03'\]"): - get_data(root, DATASET_ID) + mock_s3[(OPENNEURO_BUCKET, f"{DATASET_ID}/participants.tsv")] = ( + b"participant_id\tage\nsub-01\t20\nsub-02\t30\nsub-03\t40\n" + ) + with pytest.warns( + UserWarning, + match=r"without BIDS data: \['sub-03'\]. Excluding them from pheno.tsv\.", + ): + dpath_study = get_data(root, DATASET_ID) + pheno = (dpath_study / "tabular" / "pheno.tsv").read_text() + assert "sub-03" not in pheno + + def test_participants_empty_id( + self, + mock_s3: dict[tuple[str, str], bytes], + root: Path, + ): + mock_s3[(OPENNEURO_BUCKET, f"{DATASET_ID}/participants.tsv")] = ( + b"participant_id\tage\nsub-01\t20\nsub-02\t30\n\t\n \t\n\t\n" + ) + dpath_study = get_data(root, DATASET_ID) + pheno = (dpath_study / "tabular" / "pheno.tsv").read_text() + assert pheno == ( + "participant_id\tsession_id\tage\n" + "sub-01\tses-unnamed\t20\n" + "sub-02\tses-unnamed\t30\n" + ) def test_bids_without_participants( self, @@ -249,7 +270,6 @@ def test_single_session(self, mock_s3: dict[tuple[str, str], bytes], root: Path) pheno = (dpath_study / "tabular" / "pheno.tsv").read_text() assert "sub-01\tses-01\t" in pheno assert "sub-02\tses-02\t" in pheno - assert "sub-03\t\t" in pheno def test_multiple_sessions(self, mock_s3: dict[tuple[str, str], bytes], root: Path): mock_s3.clear() From f7729b358706dd7eeda3ff4069d58733720a2c19 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 5 Nov 2026 00:20:48 -0500 Subject: [PATCH 2/5] update docs following feedback --- docs/source/_includes/working_directory.md | 5 ++ docs/source/conf.py | 2 +- docs/source/fedbiomed_node_part_1.md | 90 ++++++++++++++++++++-- docs/source/fedbiomed_node_part_2.md | 53 ++++++++++++- docs/source/fl_ready_dataset.md | 10 ++- docs/source/freesurfer_idps.md | 37 +++++++-- docs/source/index.md | 14 +++- docs/source/initial_setup.md | 20 +++-- docs/source/phenotypic_files.md | 57 +++++++++----- docs/source/prerequisites.md | 53 ++++++++++--- 10 files changed, 287 insertions(+), 54 deletions(-) create mode 100644 docs/source/_includes/working_directory.md diff --git a/docs/source/_includes/working_directory.md b/docs/source/_includes/working_directory.md new file mode 100644 index 0000000..e685e67 --- /dev/null +++ b/docs/source/_includes/working_directory.md @@ -0,0 +1,5 @@ + +```{note} +All commands assume that the working directory is the root of the `brainflex-node` repository. +They will need to be modified if that is not the case. +``` diff --git a/docs/source/conf.py b/docs/source/conf.py index 98d2708..5a2d244 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -22,7 +22,7 @@ "sphinx_togglebutton", ] -exclude_patterns = [] +exclude_patterns = ["_includes"] nitpicky = True diff --git a/docs/source/fedbiomed_node_part_1.md b/docs/source/fedbiomed_node_part_1.md index 22f4479..4d0540a 100644 --- a/docs/source/fedbiomed_node_part_1.md +++ b/docs/source/fedbiomed_node_part_1.md @@ -4,6 +4,13 @@ We will be using [Fed-BioMed](https://fedbiomed.org/) to conduct the federated l See [this page](https://fedbiomed.org/federated-network/) of the Fed-BioMed website for more information about how it works. Essentially, each site participating in the federated experiment needs to deploy a **node**, and the person responsible for running the experiment (i.e. training the models) deploys a server (**researcher** in Fed-BioMed vocabulary) to which all nodes connect to. +During training, the researcher sends experiment-specific code (a *training plan*) to all nodes. +Nodes use this code to train a local model, then send model weights to the researcher (subject-level data never leave the node). +The researcher aggregates the set of all weights to produce a global model, which can be sent back to the nodes for further training. + +```{include} _includes/working_directory.md +``` + ## Record start time ```bash @@ -12,7 +19,7 @@ brainflex record fedbiomed_node_part_1 start ## Deploy the node -Run this to download the images (if needed) and start them: +Run this to download the images (about 18 GB in total) and start them: ```bash docker compose up -d @@ -26,19 +33,61 @@ docker compose up -d ✔ Container brainflex-node-default Started ``` +The download can take a couple of (maybe 5–10) minutes, depending on your Internet connection. + The two services are: - `vpn`: a WireGuard VPN client that connects to the central VPN server - `node`: the [node image](https://hub.docker.com/r/fedbiomed/node) distributed by Fed-BioMed -### Troubleshooting +### Troubleshooting: disk quota error + +If you run out of disk space and have already tried changing the Docker data directories [as previously described](freesurfer_idps.md#troubleshooting-disk-quota-error), you can remove the FreeSurfer image downloaded in the [IDP extraction step](freesurfer_idps), as it is not needed anymore. + +1. List your images: + + ```bash + docker image ls --filter reference=freesurfer/freesurfer:7.3.2 + ``` + + ```{code-block} + :caption: Example output + IMAGE ID DISK USAGE CONTENT SIZE EXTRA + freesurfer/freesurfer:7.3.2 e04a8cf308a8 22.5GB 6.88GB + ``` + + Note the `ID` that corresponds to the `freesurfer/freesurfer:7.3.2` image. -By default, Docker images are stored on the root disk. -If you encounter issues with disk storage quota being exceeded, you can configure Docker to use a different directory for storing data. -This can be done by following e.g. [this guide](https://www.simplified.guide/_export/xhtml/docker/change-data-directory). +2. Remove the image + + ```bash + docker image rm + ``` + +3. If you get an error about a container (running or stopped) still using the image, find it: + + ```bash + docker container ls --all --filter ancestor=freesurfer/freesurfer:7.3.2 + ``` + + Note its `ID`. + Stop it if it's running, then remove it: + + ```bash + docker container stop + docker container rm + ``` + + Then remove the image: + + ```bash + docker image rm + ``` ## Check Fed-BioMed node status +Run this to confirm that the node is running: + ```bash docker compose exec -u fedbiomed node fedbiomed node -p /fbm-node status ``` @@ -51,6 +100,21 @@ docker compose exec -u fedbiomed node fedbiomed node -p /fbm-node status Node status: NodeState.RUNNING ``` +### Troubleshooting: permission error + +If the command fails with a permission error like this: + +```{code-block} +PermissionError: [Errno 13] Permission denied: '/fbm-node/log' +``` + +check that `CONTAINER_UID` and `CONTAINER_GID` in your `.env` file match the output of `id -u` and `id -g` (see [Initial setup](initial_setup.md#env-file)), then restart the containers: + +```bash +docker compose down +docker compose up -d +``` + ## Add datasets to the node Run the two commands below to register the datasets [created by `nipoppy2fedbiomed`](fl_ready_dataset.md): @@ -66,6 +130,17 @@ Confirm that they have been added: docker compose exec -u fedbiomed node fedbiomed node -p /fbm-node dataset list ``` +You should see a table like this: + +```{code-block} +:caption: Example output +Listing your data available +name data_type tags description path shape dataset_id dataset_parameters +------ ----------- -------------------- --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ------------------------------------------------------------------------- ------------------ -------------------------------------------- -------------------- +age csv ['brainflex', 'age'] Generated by nipoppy2fedbiomed (sources: nb:Age, nb:Sex, freesurfer 7.3.2 idp/fs_stats-*/fs7.3.2-aparc.DKTatlas-thickness.tsv, freesurfer 7.3.2 idp/fs_stats-*/fs7.3.2-aseg-volume.tsv) /nipoppy_study/derivatives/nipoppy2fedbiomed/0.1.0/output/age/dataset.csv {'csv': [80, 131]} dataset_f555e76c-e98c-46f4-81f8-6936edc5a004 {} +sex csv ['brainflex', 'sex'] Generated by nipoppy2fedbiomed (sources: nb:Age, nb:Sex, freesurfer 7.3.2 idp/fs_stats-*/fs7.3.2-aparc.DKTatlas-thickness.tsv, freesurfer 7.3.2 idp/fs_stats-*/fs7.3.2-aseg-volume.tsv) /nipoppy_study/derivatives/nipoppy2fedbiomed/0.1.0/output/sex/dataset.csv {'csv': [80, 131]} dataset_4b7addc7-37c4-404c-a530-279554e3237d {} +``` + ## Share the public key with the server Run this to get the node's public key: @@ -82,6 +157,11 @@ brainflex-vpn-default | info: wireguard public key for this peer: ``` Copy the public key (or the entire output of the previous command) and send it to us via email. +We will register it on the central VPN server so that your node can join the federated network. + +```{note} +Sharing the public key is safe and necessary for establishing a connection with the VPN server. +``` ## Record end time diff --git a/docs/source/fedbiomed_node_part_2.md b/docs/source/fedbiomed_node_part_2.md index 8e51c40..0054421 100644 --- a/docs/source/fedbiomed_node_part_2.md +++ b/docs/source/fedbiomed_node_part_2.md @@ -1,5 +1,14 @@ # Fed-BioMed node: Part 2 +```{attention} +Only start this part after the FL experiment coordinator has told you that your public key has been registered and that the training plans have been sent to your node. +``` + +In this step, you will check that your node can reach the researcher, then review and approve the code that will be run on your machine. + +```{include} _includes/working_directory.md +``` + ## Record start time ```bash @@ -25,8 +34,10 @@ scripts/vpn_status ## View and approve the training plans -At this point, the Fed-BioMed researcher should have sent 2 training plans to your node. -This is the custom Python code needed for e.g. data loading and preprocessing. +A Fed-BioMed *training plan* is the custom Python code that will be run on your node, e.g. for data loading, preprocessing and model definition. + +At this point, the Fed-BioMed researcher process should have sent 2 training plans to your node (via the VPN connection), one for each prediction task (brain age regression and biological sex classification). +Your node is configured to only run approved code, so you will need to review and approve them before training can start. Run this to view the training plans (need to run once for each training plan): @@ -34,6 +45,8 @@ Run this to view the training plans (need to run once for each training plan): docker compose exec -u fedbiomed node fedbiomed node -p /fbm-node training-plan view ``` +This prints the code of the training plan to the terminal. + Once you are satisfied, run this to approve them (also need to run twice): ```bash @@ -47,3 +60,39 @@ Once both training plans are approved, the node is ready for training! ```bash brainflex record fedbiomed_node_part_2 end ``` + +## Share data about setup timing + +Timing information and your answers to the initial survey and step-specific questions have been saved to TSV files under the `.brainflex` directory: + +```bash +tree .brainflex +``` + +```{code-block} +:caption: Output +.brainflex/ +├── initial_survey.tsv +├── step_survey.tsv +└── timestamps.tsv +``` + +Take a look at them, make edits as needed, and send them to us via email. + +## Next + +Once all nodes are set up and have approved the training plans, the researcher will launch the federated learning experiment through the network. +There is nothing else to do on your side for the moment. +Just make sure to: + +- Leave the containers running +- Keep the machine turned on and connected to the internet until the end of the experiment (estimated to be 1-2 months) + +You can check that the containers are still running with: + +```bash +docker compose ps +``` + +We will keep you updated on the progress of the experiment and manuscript. +Thank you for participating! diff --git a/docs/source/fl_ready_dataset.md b/docs/source/fl_ready_dataset.md index d90ea33..392cbda 100644 --- a/docs/source/fl_ready_dataset.md +++ b/docs/source/fl_ready_dataset.md @@ -1,5 +1,10 @@ # FL-ready dataset +In this step, you will combine the standardized IDPs and the harmonized phenotypic data into one table per prediction task, **in the format expected by Fed-BioMed and the machine learning models we will train**. + +```{include} _includes/working_directory.md +``` + ## Record start time ```bash @@ -8,7 +13,8 @@ brainflex record fl_ready_data start ## Pipeline installation -Install the Nipoppy-to-Fed-BioMed conversion pipeline: +Install the [Nipoppy-to-Fed-BioMed](https://zenodo.org/records/23753437) conversion pipeline. +This pipeline was written for this FL experiment specifically. ```bash nipoppy pipeline install 23753437 --study nipoppy_study @@ -39,7 +45,7 @@ nipoppy process --pipeline nipoppy2fedbiomed --pipeline-step age --study nipoppy nipoppy process --pipeline nipoppy2fedbiomed --pipeline-step sex --study nipoppy_study ``` -For each of the prediction tasks (brain age regression and biological sex classification), this will write a CSV file (the Fed-BioMed dataset) and a JSON file used to register it on the [Fed-BioMed node](fedbiomed_node_part_1.md). +For each of the prediction tasks (brain age regression and biological sex classification), this will write a CSV file (the Fed-BioMed dataset) and a JSON file that can be used to register it on the Fed-BioMed node in the next step. ```bash tree nipoppy_study/derivatives/nipoppy2fedbiomed diff --git a/docs/source/freesurfer_idps.md b/docs/source/freesurfer_idps.md index dc4ac7f..aa28846 100644 --- a/docs/source/freesurfer_idps.md +++ b/docs/source/freesurfer_idps.md @@ -1,20 +1,33 @@ # FreeSurfer IDPs +In this step, you will use a Nipoppy pipeline to extract imaging-derived phenotypes (IDPs), such as cortical thickness and subcortical volumes, from the FreeSurfer outputs that came with your dataset. +Because every site runs the same containerized pipelines with the same configuration, **the resulting IDPs are standardized across the federation**. + +```{include} _includes/working_directory.md +``` + ## Record start time ```bash brainflex record idp_extraction start ``` +```{important} +Starting from this step, make sure to run `brainflex record` at the beginning and end of each step, as instructed. +``` + ## Install pipelines -Install the FreeSurfer and FreeSurfer stats extraction pipelines: +Install (i.e., download the JSON configuration files for) the FreeSurfer and FreeSurfer stats extraction pipelines: + +- [`freesurfer` 7.3.2](https://zenodo.org/records/21929473): FreeSurfer has already been run on your dataset, but the configuration files still need to be installed due to a quirk in how the `fs_stats` pipeline currently works in Nipoppy. +- [`fs_stats` 0.3.0](https://zenodo.org/records/23752946): an extraction pipeline that compiles FreeSurfer outputs spread across subject directories into feature tables using [`asegstats2table`](https://surfer.nmr.mgh.harvard.edu/fswiki/asegstats2table) and [`aparcstats2table`](https://surfer.nmr.mgh.harvard.edu/fswiki/aparcstats2table). ```bash -# FreeSurfer +# FreeSurfer 7.3.2 nipoppy pipeline install 21929473 --study nipoppy_study -# fs_stats +# fs_stats 0.3.0 nipoppy pipeline install 23752946 --study nipoppy_study ``` @@ -22,6 +35,9 @@ nipoppy pipeline install 23752946 --study nipoppy_study The `--study` option is not needed if your working directory is somewhere inside `nipoppy_study`. ``` +If the [FreeSurfer 7.3.2 container image](https://hub.docker.com/r/freesurfer/freesurfer/tags?name=7.3.2) is not already on your machine, you will be prompted to download it. +Enter 'y' to accept; this can take a few (maybe 5-10) minutes and will use around 23 GB of storage. + Confirm that the pipeline have been installed correctly: ```bash @@ -37,11 +53,20 @@ INFO Available extraction pipelines and versions INFO - fs_stats (0.3.0) ``` -## Configure the fs_stats pipeline with your FreeSurfer license +### Troubleshooting: disk quota error + +By default, Docker images are stored on the root disk. +If you encounter issues with disk storage quota being exceeded, you can configure Docker to use a different directory for storing data. +This can be done by following e.g. [this guide](https://www.simplified.guide/_export/xhtml/docker/change-data-directory). + +## Configure the `fs_stats` pipeline with your FreeSurfer license -The `fs_stats` pipeline requires a FreeSurfer license because it runs lightweight tools that are packaged with FreeSurfer. +The `fs_stats` pipeline requires a FreeSurfer license because it runs lightweight tools that are packaged with FreeSurfer ([`asegstats2table`](https://surfer.nmr.mgh.harvard.edu/fswiki/asegstats2table) and [`aparcstats2table`](https://surfer.nmr.mgh.harvard.edu/fswiki/aparcstats2table)). Open the study configuration file at `nipoppy_study/global_config.json` and set `FREESURFER_LICENSE_FILE` to the absolute path to your FreeSurfer license. -FreeSurfer licenses can be obtained by filling out [this form](https://surfer.nmr.mgh.harvard.edu/registration.html). + +```{note} +If you do not already have a FreeSurfer license, fill out [this form](https://surfer.nmr.mgh.harvard.edu/registration.html) to get one. +``` ```{code-block} json :emphasize-lines: 7 diff --git a/docs/source/index.md b/docs/source/index.md index 4eb2218..735278e 100644 --- a/docs/source/index.md +++ b/docs/source/index.md @@ -2,13 +2,19 @@ Real-world neuroimaging federated learning (FL) experiment using the open neuroinformatics ecosystem described in [this preprint](https://doi.org/10.64898/2026.04.30.721944). -Each site involved in the federation will: +```{admonition} What is federated learning? +[Federated learning (FL)](https://en.wikipedia.org/wiki/Federated_learning) enables the training of machine learning models on multiple datasets without direct sharing of participant-level data. +It has use-cases in neuroimaging research (and other domains) when data-sharing is not possible due to e.g. privacy or governance concerns. +``` + +This website contains instructions for sites (nodes) participating in this FL experiment. +We will help you: 1. Download an [OpenNeuro](https://openneuro.org/) dataset of ~50-100 participants that has been pre-formatted into a [Nipoppy study](https://nipoppy.readthedocs.io/en/latest/) with (already processed) FreeSurfer derivatives and demographics (age and sex) -2. Run some fast (no HPC required), user-friendly [Nipoppy](https://nipoppy.readthedocs.io/en/latest/) and [Neurobagel](https://neurobagel.org/) tools to obtain FL-ready data -3. Deploy a [Fed-BioMed](https://fedbiomed.org/) client node and participate in the training of simple machine learning models for brain age regression and biological sex classification from FreeSurfer cortical thickness/subcortical volume data +2. Use some fast (no HPC required), user-friendly [Nipoppy](https://nipoppy.readthedocs.io/en/latest/) and [Neurobagel](https://neurobagel.org/) tools to obtain **FL-ready data** +3. Deploy a [Fed-BioMed](https://fedbiomed.org/) client node and participate in the training of simple machine learning models for **brain age regression** and **biological sex classification** from FreeSurfer cortical thickness/subcortical volume data -The time required to do each step will be recorded and included in the manuscript along with the model results. +Information about the perceived usefulness of the tools and the time required to do each step will be recorded and included in the manuscript along with the model results. ```{toctree} --- diff --git a/docs/source/initial_setup.md b/docs/source/initial_setup.md index cd67f3f..97159dd 100644 --- a/docs/source/initial_setup.md +++ b/docs/source/initial_setup.md @@ -4,7 +4,7 @@ ### Clone the starting repository -Clone the [`brainflex-node`](https://github.com/neurodatascience/brainflex-node) repository: +Clone and enter the [`brainflex-node`](https://github.com/neurodatascience/brainflex-node) repository: ```bash git clone https://github.com/neurodatascience/brainflex-node @@ -15,6 +15,8 @@ cd brainflex-node #### With `uv` +To install `uv`: [https://docs.astral.sh/uv/getting-started/installation/](https://docs.astral.sh/uv/getting-started/installation/) (`sudo` privileges **not** needed). + ```bash uv venv --python 3.14 source .venv/bin/activate @@ -61,7 +63,7 @@ brainflex initial-survey ## Node configuration ```{attention} -`config.env` and `.env` are Git-ignored and should never be shared. +The content of `config.env` and `.env` should not be openly shared. ``` ### `config.env` file @@ -69,7 +71,7 @@ brainflex initial-survey Put the `config.env` file that was shared with you in the `vpn_config/` directory: ```bash -mv vpn_config/config.env +mv vpn_config/config.env ``` ### `.env` file @@ -104,7 +106,7 @@ FBM_CONTAINER_VERSION_TAG=6.4.1 FBM_CONTAINER_INSTANCE_ID=default # Fed-BioMed node name, as shown to the researcher -FBM_NODE_NAME=mcgill +FBM_NODE_NAME=mcgill-ds123456 # Host user and group owning the files in `fbm-node` # Set to the output of `id -u` and `id -g`. If unset, files are owned by UID/GID 2101 @@ -133,10 +135,16 @@ FBM_SECURITY_ALLOW_FEDERATED_ANALYTICS=True ## Data download -Download the OpenNeuro dataset assigned to your site, replacing `` with its ID (beginning with `ds`): +Download the OpenNeuro dataset assigned to your site, replacing `` with its ID. +Your dataset ID is the second part of the `FBM_NODE_NAME` variable you just set in your `.env` file, beginning with `ds` (e.g. `ds123456` in the example `.env` file above). ```bash brainflex get-data ``` -This creates a minimal Nipoppy study in `nipoppy_study` that contains all the data needed for the federated learning experiment. +The command may run for a few (maybe 3-5) minutes. +It will create a minimal Nipoppy study in a new directory, `nipoppy_study`, that contains all the data needed for the federated learning experiment. + +```{note} +To save disk space and download time, only the files needed for this experiment are downloaded: the raw imaging files are empty placeholders, and the FreeSurfer outputs only include the `stats` and `scripts` directories. +``` diff --git a/docs/source/phenotypic_files.md b/docs/source/phenotypic_files.md index cdb6cad..787b04f 100644 --- a/docs/source/phenotypic_files.md +++ b/docs/source/phenotypic_files.md @@ -1,31 +1,47 @@ # Phenotypic files +In this step, you will use the [Neurobagel annotation tool](https://annotate.neurobagel.org/) to map your dataset's phenotypic variables (age and sex) from their original naming/encoding conventions to standardized terms. +You will then create a semantically harmonized copy of your phenotypic data, where column names and values use a **standardized vocabulary** that is shared with all other sites. + +```{include} _includes/working_directory.md +``` + ## Record start time ```bash brainflex record semantic_harmonization start ``` -## Annotation +## Annotate phenotypic TSV file + +The [Neurobagel annotation tool](https://annotate.neurobagel.org/) runs in a web browser. +You need to upload: + +- `nipoppy_study/tabular/pheno.tsv` in the **Data Table** box +- `nipoppy_study/tabular/pheno.json` in the **Data Dictionary** box (optional, only if present) -Download the following files to your local machine: +If you are working on a remote machine, you will need to first copy these files to the computer where you use your web browser (e.g. with `scp`). -- `nipoppy_study/tabular/pheno.tsv` -- `nipoppy_study/tabular/pheno.json` (if present) +Then, go through the annotation tool workflow at [https://annotate.neurobagel.org/](https://annotate.neurobagel.org/) using these files. -Then, go through the [Neurobagel annotation tool](https://annotate.neurobagel.org/) workflow using these files. -The following columns need to be annotated: +```{tip} +Click on the "How to use this page?" buttons in the annotation tool for guidance. +``` + +The following standardized variables **need to be assigned**: - Participant ID - Session ID - Age - Sex -Save the annotated data dictionary to `nipoppy_study/tabular/pheno_annotated.json`. +You can ignore the "Diagnosis" and "Assessment Tool" standardized variables, as well as the "Data Description" step near the end of the workflow. + +Save/copy the annotated data dictionary to `nipoppy_study/tabular/pheno_annotated.json` on the machine with the rest of the data. -## Harmonization +## Create harmonized phenotypic TSV file -Harmonize the phenotypic data using Neurobagel's CLI: +Harmonize the phenotypic data using the Neurobagel CLI: ```bash bagel harmonize-pheno \ @@ -44,18 +60,23 @@ head nipoppy_study/tabular/harmonized.tsv ```{code-block} :caption: Example output nb:ParticipantID nb:SessionID nb:Sex nb:Age -sub-1 ses-unnamed snomed:248153007 20.0 -sub-2 ses-unnamed snomed:248152002 25.0 -sub-3 ses-unnamed snomed:248153007 30.0 -sub-4 ses-unnamed snomed:248152002 35.0 -sub-5 ses-unnamed snomed:248153007 40.0 -sub-6 ses-unnamed snomed:248152002 45.0 -sub-7 ses-unnamed snomed:248153007 50.0 -sub-8 ses-unnamed snomed:248152002 55.0 -sub-9 ses-unnamed snomed:248153007 60.0 +sub-1 ses-1 snomed:248153007 20.0 +sub-2 ses-1 snomed:248152002 25.0 +sub-3 ses-1 snomed:248153007 30.0 +sub-4 ses-1 snomed:248152002 35.0 +sub-5 ses-1 snomed:248153007 40.0 +sub-6 ses-1 snomed:248152002 45.0 +sub-7 ses-1 snomed:248153007 50.0 +sub-8 ses-1 snomed:248152002 55.0 +sub-9 ses-1 snomed:248153007 60.0 ``` +```{note} +You may have `ses-unnamed` in the `nb:SessionID` column. +This is because some OpenNeuro datasets do not have explicit session names, in which case Nipoppy will use `unnamed` as the session ID. +``` + ## Record end time and answer questions ```bash diff --git a/docs/source/prerequisites.md b/docs/source/prerequisites.md index 1ddd7ee..b698181 100644 --- a/docs/source/prerequisites.md +++ b/docs/source/prerequisites.md @@ -2,7 +2,11 @@ ## Docker and Docker Compose -Make sure your Docker installation is working: +In order to deploy a node for the federated learning experiment, you will need [Docker](https://www.docker.com/) and [Docker Compose](https://docs.docker.com/compose/). +Docker is a tool that runs software inside containers, which are self-contained packages that bundle a program with all its dependencies. +Docker Compose builds on Docker to start and manage several containers together from a single configuration file (`docker-compose.yml`). + +Check if Docker is installed and working: ```bash docker run hello-world @@ -35,8 +39,29 @@ For more examples and ideas, visit: https://docs.docker.com/get-started/ ``` +````{note} +You can check your Docker version with: + +```bash +docker version --format '{{.Server.Version}}' +``` + +```{code-block} +:caption: Example output +29.7.0 +``` + +Any recent version of Docker should work. +These instructions were tested with Docker 29.1.3, 29.6.2 and 29.7.0. +```` + If Docker is not installed, see [these instructions](https://docs.docker.com/engine/install/) to install it and make sure to follow the [post-setup instructions](https://docs.docker.com/engine/install/linux-postinstall/). +```{attention} +Installing Docker and Docker Compose requires `sudo` privileges. +If you do not have `sudo` privileges, you will need to ask a sys admin or your IT department to do the installation for you. +``` + Also make sure Docker Compose is installed: ```bash @@ -46,15 +71,18 @@ docker compose version You should get something like this: ```{code-block} -Docker Compose version vX.Y.Z +:caption: Example output +Docker Compose version v5.3.1 ``` -```{attention} -Installing Docker and Docker Compose requires `sudo` privileges. -If you do not have `sudo` privileges, you will need to ask a system admin do to the installation for you. +```{note} +Any recent version of Docker Compose should work. +These instructions were tested with versions 2.37.1 and 5.3.1. ``` -### Troubleshooting +If Docker Compose is not installed, follow [these instructions](https://docs.docker.com/compose/install/linux/#install-using-the-repository) (or ask a sys admin) to install it. + +### Troubleshooting: permission error If you get a `permission denied` error, check if your user is in the `docker` group: @@ -62,9 +90,14 @@ If you get a `permission denied` error, check if your user is in the `docker` gr groups ``` -If `docker` is not in any of the groups listed, follow [this guide](https://docs.docker.com/engine/install/linux-postinstall/#add-your-user-to-the-docker-group) to add it. +If `docker` is not in any of the groups listed, follow [this guide](https://docs.docker.com/engine/install/linux-postinstall/#add-your-user-to-the-docker-group) (or ask your sys admin) to add it. + +## Networking -## Network prerequisite +Each site connects to the federated network through a [WireGuard](https://www.wireguard.com/) virtual private network (VPN). -In order to join the federated network, the machine needs to have outbound UDP access to an external server (see `VPN_SERVER_ENDPOINT` in `config.env` file). -Replies from the server must also be allowed back in. +In order to join the federated network, your machine needs outbound UDP access to an external server, on the host and port given by `VPN_SERVER_ENDPOINT` in the `config.env` file. No inbound ports need to be opened. + +```{note} +[UDP](https://en.wikipedia.org/wiki/User_Datagram_Protocol) is one of the two main protocols used to send data over the internet (the other being [TCP](https://en.wikipedia.org/wiki/Transmission_Control_Protocol)). Outbound UDP traffic is allowed by default on many institutional networks. +``` From 9f87aca9c983acb4fd79eabad7c20e31b9fe5577 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 5 Nov 2026 00:32:49 -0500 Subject: [PATCH 3/5] make doc page name consistent with step name --- docs/source/fedbiomed_node_part_1.md | 2 +- docs/source/fl_ready_dataset.md | 4 ++-- .../{freesurfer_idps.md => idp_extraction.md} | 2 +- docs/source/index.md | 4 ++-- ...notypic_files.md => semantic_harmonization.md} | 2 +- src/brainflex/core/record.py | 6 +++--- tests/cli/test_record.py | 15 ++++++++------- tests/core/test_record.py | 8 ++++---- 8 files changed, 22 insertions(+), 21 deletions(-) rename docs/source/{freesurfer_idps.md => idp_extraction.md} (99%) rename docs/source/{phenotypic_files.md => semantic_harmonization.md} (99%) diff --git a/docs/source/fedbiomed_node_part_1.md b/docs/source/fedbiomed_node_part_1.md index 4d0540a..57b565c 100644 --- a/docs/source/fedbiomed_node_part_1.md +++ b/docs/source/fedbiomed_node_part_1.md @@ -42,7 +42,7 @@ The two services are: ### Troubleshooting: disk quota error -If you run out of disk space and have already tried changing the Docker data directories [as previously described](freesurfer_idps.md#troubleshooting-disk-quota-error), you can remove the FreeSurfer image downloaded in the [IDP extraction step](freesurfer_idps), as it is not needed anymore. +If you run out of disk space and have already tried changing the Docker data directories [as previously described](idp_extraction.md#troubleshooting-disk-quota-error), you can remove the FreeSurfer image downloaded in the [IDP extraction step](idp_extraction), as it is not needed anymore. 1. List your images: diff --git a/docs/source/fl_ready_dataset.md b/docs/source/fl_ready_dataset.md index 392cbda..2e347ea 100644 --- a/docs/source/fl_ready_dataset.md +++ b/docs/source/fl_ready_dataset.md @@ -8,7 +8,7 @@ In this step, you will combine the standardized IDPs and the harmonized phenotyp ## Record start time ```bash -brainflex record fl_ready_data start +brainflex record fl_ready_dataset start ``` ## Pipeline installation @@ -67,5 +67,5 @@ nipoppy_study/derivatives/nipoppy2fedbiomed ## Record end time and answer questions ```bash -brainflex record fl_ready_data end +brainflex record fl_ready_dataset end ``` diff --git a/docs/source/freesurfer_idps.md b/docs/source/idp_extraction.md similarity index 99% rename from docs/source/freesurfer_idps.md rename to docs/source/idp_extraction.md index aa28846..6004a71 100644 --- a/docs/source/freesurfer_idps.md +++ b/docs/source/idp_extraction.md @@ -1,4 +1,4 @@ -# FreeSurfer IDPs +# IDP extraction In this step, you will use a Nipoppy pipeline to extract imaging-derived phenotypes (IDPs), such as cortical thickness and subcortical volumes, from the FreeSurfer outputs that came with your dataset. Because every site runs the same containerized pipelines with the same configuration, **the resulting IDPs are standardized across the federation**. diff --git a/docs/source/index.md b/docs/source/index.md index 735278e..3016fdb 100644 --- a/docs/source/index.md +++ b/docs/source/index.md @@ -25,8 +25,8 @@ caption: Instructions --- prerequisites initial_setup -freesurfer_idps -phenotypic_files +idp_extraction +semantic_harmonization fl_ready_dataset fedbiomed_node_part_1 fedbiomed_node_part_2 diff --git a/docs/source/phenotypic_files.md b/docs/source/semantic_harmonization.md similarity index 99% rename from docs/source/phenotypic_files.md rename to docs/source/semantic_harmonization.md index 787b04f..7de7da2 100644 --- a/docs/source/phenotypic_files.md +++ b/docs/source/semantic_harmonization.md @@ -1,4 +1,4 @@ -# Phenotypic files +# Semantic harmonization In this step, you will use the [Neurobagel annotation tool](https://annotate.neurobagel.org/) to map your dataset's phenotypic variables (age and sex) from their original naming/encoding conventions to standardized terms. You will then create a semantically harmonized copy of your phenotypic data, where column names and values use a **standardized vocabulary** that is shared with all other sites. diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 861e35a..8641a3f 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -31,7 +31,7 @@ class Step(StrEnum): IDP_EXTRACTION = "idp_extraction" SEMANTIC_HARMONIZATION = "semantic_harmonization" - FL_READY_DATA = "fl_ready_data" + FL_READY_DATASET = "fl_ready_dataset" FEDBIOMED_NODE_PART_1 = "fedbiomed_node_part_1" FEDBIOMED_NODE_PART_2 = "fedbiomed_node_part_2" @@ -44,7 +44,7 @@ class Status(StrEnum): DATA_PREP_STEPS = frozenset( - {Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATA} + {Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATASET} ) # questions asked at the end of a step, mapped to their allowed answers @@ -71,7 +71,7 @@ class Status(StrEnum): "despite the fact that the original demographic data may have used " "different names or values." ), - Step.FL_READY_DATA: ( + Step.FL_READY_DATASET: ( "In this step, you merged the previously extracted FreeSurfer IDPs and " "harmonized phenotypic data into datasets formatted in a way that can be used " "directly by the Fed-BioMed federated learning framework. All sites " diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index d5cb019..074ffe0 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -91,7 +91,8 @@ def test_end_other( ) @pytest.mark.parametrize( - "step", [Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATA] + "step", + [Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATASET], ) def test_end_data_prep( self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path, step: Step @@ -256,7 +257,7 @@ def test_warning_not_started_printed_first( mock_record.assert_called_once() def test_end_to_end(self, runner: CliRunner, dpath_root: Path): - args = ["record", "fl_ready_data", "--project", str(dpath_root)] + args = ["record", "fl_ready_dataset", "--project", str(dpath_root)] args_start = [*args[:2], "start", *args[2:]] args_end = [*args[:2], "end", *args[2:]] dpath = dpath_root.resolve() / BRAINFLEX_DIRNAME @@ -264,7 +265,7 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): assert result.exit_code == 0 fpath_timestamps = dpath / TIMESTAMPS_FILENAME assert ( - f"Saved timestamp for (fl_ready_data, start) to {fpath_timestamps}" + f"Saved timestamp for (fl_ready_dataset, start) to {fpath_timestamps}" in result.stdout ) assert "Saved answers" not in result.stdout @@ -272,7 +273,7 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): assert "WARNING" not in result.stderr assert result.exit_code == 0 assert ( - f"Saved timestamp for (fl_ready_data, end) to {fpath_timestamps}" + f"Saved timestamp for (fl_ready_dataset, end) to {fpath_timestamps}" in result.stdout ) assert f"Saved answers to {dpath / STEP_SURVEY_FILENAME}" in result.stdout @@ -281,7 +282,7 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): assert list(df_timestamps["status"]) == ["start", "end"] assert (dpath / STEP_SURVEY_FILENAME).read_text().splitlines() == [ "step\tquestion\tanswer", - f"fl_ready_data\t{QUESTION_TOOLS_EASIER}\tyes", - f"fl_ready_data\t{QUESTION_TIME_WITHOUT_TOOLS}\t<1h", - f"fl_ready_data\t{QUESTION_NOTES}\tok", + f"fl_ready_dataset\t{QUESTION_TOOLS_EASIER}\tyes", + f"fl_ready_dataset\t{QUESTION_TIME_WITHOUT_TOOLS}\t<1h", + f"fl_ready_dataset\t{QUESTION_NOTES}\tok", ] diff --git a/tests/core/test_record.py b/tests/core/test_record.py index e5f9a60..303f49f 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -126,10 +126,10 @@ def test_end_other(self, root: Path, fpath_timestamps: Path, fpath_survey: Path) ] def test_survey_appends_in_question_order(self, root: Path, fpath_survey: Path): - record(root, Step.FL_READY_DATA, Status.START) + record(root, Step.FL_READY_DATASET, Status.START) record( root, - Step.FL_READY_DATA, + Step.FL_READY_DATASET, Status.END, answers=dict(reversed(DATA_PREP_ANSWERS.items())), ) @@ -266,8 +266,8 @@ def test_warning( def test_other_step(self, root: Path): self._record(root, Step.IDP_EXTRACTION, Status.START) - with pytest.warns(UserWarning, match="fl_ready_data was not started"): - check_status(root, Step.FL_READY_DATA, Status.END) + with pytest.warns(UserWarning, match="fl_ready_dataset was not started"): + check_status(root, Step.FL_READY_DATASET, Status.END) def test_invalid_timestamps_file(self, root: Path, fpath_timestamps: Path): fpath_timestamps.write_text("timestamp\tstep\nt0\tidp_extraction\n") From 6e6137a4a56ff91f0db8dda5f2c33a640e95ce67 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 5 Nov 2026 00:53:16 -0500 Subject: [PATCH 4/5] make step questions refer to the single tool used in that step --- src/brainflex/cli/record.py | 2 +- src/brainflex/core/record.py | 39 ++++++++++----- src/brainflex/utils/constants.py | 6 +-- tests/cli/test_record.py | 47 +++++++++++------- tests/core/test_record.py | 82 +++++++++++++++++++++++--------- 5 files changed, 122 insertions(+), 54 deletions(-) diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index 55862ce..fc01f46 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -48,6 +48,6 @@ def record_cli( fpath_timestamps, fpath_survey = record( path, step, status, answers=answers, timestamp=timestamp ) - typer.echo(f"Saved timestamp for ({step}, {status}) to {fpath_timestamps}") + typer.echo(f"\nSaved timestamp for ({step}, {status}) to {fpath_timestamps}") if fpath_survey is not None: typer.echo(f"Saved answers to {fpath_survey}") diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 8641a3f..28f32fc 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -15,7 +15,7 @@ COL_TIMESTAMP, QUESTION_NOTES, QUESTION_TIME_WITHOUT_TOOLS, - QUESTION_TOOLS_EASIER, + QUESTION_TOOL_EASIER_TEMPLATE, STEP_SURVEY_COLUMNS, STEP_SURVEY_FILENAME, TIME_WITHOUT_TOOLS_CHOICES, @@ -47,32 +47,33 @@ class Status(StrEnum): {Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATASET} ) -# questions asked at the end of a step, mapped to their allowed answers -# (None for free text), in the order they are asked -DATA_PREP_QUESTIONS: dict[str, tuple[str, ...] | None] = { - QUESTION_TOOLS_EASIER: TOOLS_EASIER_CHOICES, - QUESTION_TIME_WITHOUT_TOOLS: TIME_WITHOUT_TOOLS_CHOICES, +# tool used in each data preparation step +STEP_TOOLS: dict[Step, str] = { + Step.IDP_EXTRACTION: "Nipoppy", + Step.SEMANTIC_HARMONIZATION: "Neurobagel", + Step.FL_READY_DATASET: "Nipoppy", } + COMMON_QUESTIONS: dict[str, tuple[str, ...] | None] = {QUESTION_NOTES: None} # high-level summaries of data preparation steps STEP_SUMMARIES: dict[Step, str] = { Step.IDP_EXTRACTION: ( - "In this step, you used a script that wraps FreeSurfer tools to:\n- extract " + "\nIn this step, you used a script that wraps FreeSurfer tools to:\n- extract " "imaging-derived phenotypes (IDPs) (cortical thickness, subcortical volumes, " "etc.) from existing FreeSurfer subject directories\n- combine them into " "tabular files with standardized structure and column names (shared with every " "other site participating in the FL experiment)" ), Step.SEMANTIC_HARMONIZATION: ( - "In this step, you created a file containing participant IDs, session IDs, and " + "\nIn this step, you created a file containing participant IDs, session IDs, and " "demographic information (age, sex) that use standardized column names and " "values (shared with every other site participating in the FL experiment), " "despite the fact that the original demographic data may have used " "different names or values." ), Step.FL_READY_DATASET: ( - "In this step, you merged the previously extracted FreeSurfer IDPs and " + "\nIn this step, you merged the previously extracted FreeSurfer IDPs and " "harmonized phenotypic data into datasets formatted in a way that can be used " "directly by the Fed-BioMed federated learning framework. All sites " "participating in the FL experiment will have datasets with the same structure " @@ -81,10 +82,26 @@ class Status(StrEnum): } +def get_tools_easier_question(step: Step) -> str: + """Return the question about whether the tool made a data preparation step easier.""" + return QUESTION_TOOL_EASIER_TEMPLATE.format(tool=STEP_TOOLS[step]) + + +def get_time_without_tools_question(step: Step) -> str: + """Return the question about how long a data preparation step would have taken.""" + return QUESTION_TIME_WITHOUT_TOOLS.format(tool=STEP_TOOLS[step]) + + def get_step_questions(step: Step) -> dict[str, tuple[str, ...] | None]: - """Return the questions asked at the end of a step, mapped to allowed answers.""" + """Return the questions asked at the end of a step, mapped to allowed answers. + + Allowed answers are None for free text. + """ if step in DATA_PREP_STEPS: - return DATA_PREP_QUESTIONS | COMMON_QUESTIONS + return { + get_tools_easier_question(step): TOOLS_EASIER_CHOICES, + get_time_without_tools_question(step): TIME_WITHOUT_TOOLS_CHOICES, + } | COMMON_QUESTIONS return dict(COMMON_QUESTIONS) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index d15b033..3291bbc 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -33,12 +33,10 @@ TIMESTAMPS_COLUMNS = (COL_TIMESTAMP, COL_STEP, COL_STATUS) STEP_SURVEY_FILENAME = "step_survey.tsv" STEP_SURVEY_COLUMNS = (COL_STEP, *SURVEY_COLUMNS) -QUESTION_TOOLS_EASIER = ( - "Did the Nipoppy/Neurobagel tools make things easier to do this step?" -) +QUESTION_TOOL_EASIER_TEMPLATE = "Did {tool} make it easier to do this step?" TOOLS_EASIER_CHOICES = ("yes", "no", "unsure") QUESTION_TIME_WITHOUT_TOOLS = ( - "How long do you think this would have taken without the Nipoppy/Neurobagel tools?" + "How long do you think this would have taken without {tool}?" ) TIME_WITHOUT_TOOLS_CHOICES = ("<1h", "1-4h", "4-8h", "1-3d", "3-7d", ">7d") QUESTION_NOTES = "Notes/feedback" diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index 074ffe0..14e6379 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -7,12 +7,15 @@ from typer.testing import CliRunner from brainflex.cli.main import app -from brainflex.core.record import Status, Step +from brainflex.core.record import ( + Status, + Step, + get_time_without_tools_question, + get_tools_easier_question, +) from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, QUESTION_NOTES, - QUESTION_TIME_WITHOUT_TOOLS, - QUESTION_TOOLS_EASIER, STEP_SURVEY_FILENAME, TIMESTAMPS_FILENAME, ) @@ -75,8 +78,8 @@ def test_end_other( assert result.exit_code == 0 stdout = _unwrap(result.stdout) assert QUESTION_NOTES in stdout - assert QUESTION_TOOLS_EASIER not in stdout - assert QUESTION_TIME_WITHOUT_TOOLS not in stdout + assert "make it easier" not in stdout + assert "would have taken" not in stdout assert ( f"Saved timestamp for (fedbiomed_node_part_1, end) to {FPATH_TIMESTAMPS}" in result.stdout @@ -91,11 +94,20 @@ def test_end_other( ) @pytest.mark.parametrize( - "step", - [Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATASET], + ("step", "tool"), + [ + (Step.IDP_EXTRACTION, "Nipoppy"), + (Step.SEMANTIC_HARMONIZATION, "Neurobagel"), + (Step.FL_READY_DATASET, "Nipoppy"), + ], ) def test_end_data_prep( - self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path, step: Step + self, + runner: CliRunner, + mock_record: MagicMock, + dpath_root: Path, + step: Step, + tool: str, ): result = runner.invoke( app, @@ -106,8 +118,8 @@ def test_end_data_prep( stdout = _unwrap(result.stdout) # questions are asked in order, with the free-form question last assert ( - stdout.index(QUESTION_TOOLS_EASIER) - < stdout.index(QUESTION_TIME_WITHOUT_TOOLS) + stdout.index(f"Did {tool} make it easier to do this step?") + < stdout.index(f"would have taken without {tool}?") < stdout.index(QUESTION_NOTES) ) assert "[yes/no/unsure]" in stdout @@ -120,8 +132,8 @@ def test_end_data_prep( step, Status.END, answers={ - QUESTION_TOOLS_EASIER: "yes", - QUESTION_TIME_WITHOUT_TOOLS: "1-4h", + get_tools_easier_question(step): "yes", + get_time_without_tools_question(step): "1-4h", QUESTION_NOTES: "went fine", }, timestamp=ANY, @@ -141,8 +153,8 @@ def test_end_data_prep_invalid_input_reprompts( Step.IDP_EXTRACTION, Status.END, answers={ - QUESTION_TOOLS_EASIER: "unsure", - QUESTION_TIME_WITHOUT_TOOLS: ">7d", + get_tools_easier_question(Step.IDP_EXTRACTION): "unsure", + get_time_without_tools_question(Step.IDP_EXTRACTION): ">7d", QUESTION_NOTES: "", }, timestamp=ANY, @@ -282,7 +294,10 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): assert list(df_timestamps["status"]) == ["start", "end"] assert (dpath / STEP_SURVEY_FILENAME).read_text().splitlines() == [ "step\tquestion\tanswer", - f"fl_ready_dataset\t{QUESTION_TOOLS_EASIER}\tyes", - f"fl_ready_dataset\t{QUESTION_TIME_WITHOUT_TOOLS}\t<1h", + "fl_ready_dataset\tDid Nipoppy make it easier to do this step?\tyes", + ( + "fl_ready_dataset\tHow long do you think this would have taken" + " without Nipoppy?\t<1h" + ), f"fl_ready_dataset\t{QUESTION_NOTES}\tok", ] diff --git a/tests/core/test_record.py b/tests/core/test_record.py index 303f49f..f4e8416 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -1,4 +1,5 @@ import warnings +from collections.abc import Callable from datetime import datetime, timedelta, timezone from pathlib import Path @@ -11,24 +12,26 @@ Step, check_status, get_step_questions, + get_time_without_tools_question, + get_tools_easier_question, record, ) from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, QUESTION_NOTES, - QUESTION_TIME_WITHOUT_TOOLS, - QUESTION_TOOLS_EASIER, STEP_SURVEY_COLUMNS, STEP_SURVEY_FILENAME, TIMESTAMPS_COLUMNS, TIMESTAMPS_FILENAME, ) -DATA_PREP_ANSWERS = { - QUESTION_TOOLS_EASIER: "yes", - QUESTION_TIME_WITHOUT_TOOLS: "1-4h", - QUESTION_NOTES: "went fine", -} + +def _data_prep_answers(step: Step) -> dict[str, str]: + return { + get_tools_easier_question(step): "yes", + get_time_without_tools_question(step): "1-4h", + QUESTION_NOTES: "went fine", + } @pytest.fixture @@ -55,11 +58,25 @@ class TestGetStepQuestions: @pytest.mark.parametrize("step", sorted(DATA_PREP_STEPS)) def test_data_prep(self, step: Step): assert list(get_step_questions(step)) == [ - QUESTION_TOOLS_EASIER, - QUESTION_TIME_WITHOUT_TOOLS, + get_tools_easier_question(step), + get_time_without_tools_question(step), QUESTION_NOTES, ] + @pytest.mark.parametrize( + ("step", "tool"), + [ + (Step.IDP_EXTRACTION, "Nipoppy"), + (Step.SEMANTIC_HARMONIZATION, "Neurobagel"), + (Step.FL_READY_DATASET, "Nipoppy"), + ], + ) + def test_data_prep_tool(self, step: Step, tool: str): + assert list(get_step_questions(step))[:2] == [ + f"Did {tool} make it easier to do this step?", + f"How long do you think this would have taken without {tool}?", + ] + @pytest.mark.parametrize("step", sorted(set(Step) - DATA_PREP_STEPS)) def test_other(self, step: Step): assert list(get_step_questions(step)) == [QUESTION_NOTES] @@ -92,7 +109,10 @@ def test_end_data_prep( ): record(root, Step.IDP_EXTRACTION, Status.START) assert record( - root, Step.IDP_EXTRACTION, Status.END, answers=DATA_PREP_ANSWERS + root, + Step.IDP_EXTRACTION, + Status.END, + answers=_data_prep_answers(Step.IDP_EXTRACTION), ) == (fpath_timestamps, fpath_survey) assert list(_read(fpath_timestamps)["status"]) == ["start", "end"] assert fpath_timestamps.read_text().count("timestamp") == 1 @@ -101,12 +121,14 @@ def test_end_data_prep( assert df.to_dict("records") == [ { "step": "idp_extraction", - "question": QUESTION_TOOLS_EASIER, + "question": "Did Nipoppy make it easier to do this step?", "answer": "yes", }, { "step": "idp_extraction", - "question": QUESTION_TIME_WITHOUT_TOOLS, + "question": ( + "How long do you think this would have taken without Nipoppy?" + ), "answer": "1-4h", }, { @@ -126,19 +148,20 @@ def test_end_other(self, root: Path, fpath_timestamps: Path, fpath_survey: Path) ] def test_survey_appends_in_question_order(self, root: Path, fpath_survey: Path): + answers = _data_prep_answers(Step.FL_READY_DATASET) record(root, Step.FL_READY_DATASET, Status.START) record( root, Step.FL_READY_DATASET, Status.END, - answers=dict(reversed(DATA_PREP_ANSWERS.items())), + answers=dict(reversed(answers.items())), ) record(root, Step.FEDBIOMED_NODE_PART_2, Status.START) record( root, Step.FEDBIOMED_NODE_PART_2, Status.END, answers={QUESTION_NOTES: "x"} ) df = _read(fpath_survey) - assert list(df["question"]) == [*DATA_PREP_ANSWERS, QUESTION_NOTES] + assert list(df["question"]) == [*answers, QUESTION_NOTES] assert fpath_survey.read_text().count("question") == 1 def test_notes_with_special_characters(self, root: Path, fpath_survey: Path): @@ -158,7 +181,17 @@ def test_notes_with_special_characters(self, root: Path, fpath_survey: Path): (Step.IDP_EXTRACTION, Status.START, {QUESTION_NOTES: ""}), (Step.IDP_EXTRACTION, Status.END, None), (Step.IDP_EXTRACTION, Status.END, {QUESTION_NOTES: ""}), - (Step.FEDBIOMED_NODE_PART_1, Status.END, DATA_PREP_ANSWERS), + ( + Step.FEDBIOMED_NODE_PART_1, + Status.END, + _data_prep_answers(Step.IDP_EXTRACTION), + ), + # questions name the tool used in a different step + ( + Step.SEMANTIC_HARMONIZATION, + Status.END, + _data_prep_answers(Step.IDP_EXTRACTION), + ), (Step.FEDBIOMED_NODE_PART_1, Status.END, {"Unknown?": ""}), ], ) @@ -177,22 +210,27 @@ def test_invalid_questions( assert not fpath_survey.exists() @pytest.mark.parametrize( - ("question", "answer"), + ("get_question", "answer"), [ - (QUESTION_TOOLS_EASIER, "maybe"), - (QUESTION_TOOLS_EASIER, "4"), - (QUESTION_TIME_WITHOUT_TOOLS, "2 days"), + (get_tools_easier_question, "maybe"), + (get_tools_easier_question, "4"), + (get_time_without_tools_question, "2 days"), ], ) def test_invalid_answer( - self, root: Path, fpath_timestamps: Path, question: str, answer: str + self, + root: Path, + fpath_timestamps: Path, + get_question: Callable[[Step], str], + answer: str, ): + step = Step.SEMANTIC_HARMONIZATION with pytest.raises(ValueError, match="must be one of"): record( root, - Step.SEMANTIC_HARMONIZATION, + step, Status.END, - answers={**DATA_PREP_ANSWERS, question: answer}, + answers={**_data_prep_answers(step), get_question(step): answer}, ) assert not fpath_timestamps.exists() From a69cbb1e3e7249ca2e8573717a703d254331f4ed Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 5 Nov 2026 00:57:14 -0500 Subject: [PATCH 5/5] rename constant --- src/brainflex/core/record.py | 4 ++-- src/brainflex/utils/constants.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 28f32fc..277bdbe 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -14,7 +14,7 @@ COL_STEP, COL_TIMESTAMP, QUESTION_NOTES, - QUESTION_TIME_WITHOUT_TOOLS, + QUESTION_TIME_WITHOUT_TOOL_TEMPLATE, QUESTION_TOOL_EASIER_TEMPLATE, STEP_SURVEY_COLUMNS, STEP_SURVEY_FILENAME, @@ -89,7 +89,7 @@ def get_tools_easier_question(step: Step) -> str: def get_time_without_tools_question(step: Step) -> str: """Return the question about how long a data preparation step would have taken.""" - return QUESTION_TIME_WITHOUT_TOOLS.format(tool=STEP_TOOLS[step]) + return QUESTION_TIME_WITHOUT_TOOL_TEMPLATE.format(tool=STEP_TOOLS[step]) def get_step_questions(step: Step) -> dict[str, tuple[str, ...] | None]: diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index 3291bbc..8aa68dd 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -35,7 +35,7 @@ STEP_SURVEY_COLUMNS = (COL_STEP, *SURVEY_COLUMNS) QUESTION_TOOL_EASIER_TEMPLATE = "Did {tool} make it easier to do this step?" TOOLS_EASIER_CHOICES = ("yes", "no", "unsure") -QUESTION_TIME_WITHOUT_TOOLS = ( +QUESTION_TIME_WITHOUT_TOOL_TEMPLATE = ( "How long do you think this would have taken without {tool}?" ) TIME_WITHOUT_TOOLS_CHOICES = ("<1h", "1-4h", "4-8h", "1-3d", "3-7d", ">7d")