From 4544d74201c1682eb1dd8893687687dd891d3d96 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 16:12:02 -0500 Subject: [PATCH 01/16] initial pass --- docs/source/initial_setup.md | 8 ++ src/brainflex/cli/initial_survey.py | 38 ++++++++++ src/brainflex/cli/main.py | 2 + src/brainflex/core/initial_survey.py | 65 ++++++++++++++++ src/brainflex/utils/constants.py | 13 ++++ tests/cli/test_initial_survey.py | 107 +++++++++++++++++++++++++++ tests/core/test_initial_survey.py | 75 +++++++++++++++++++ 7 files changed, 308 insertions(+) create mode 100644 src/brainflex/cli/initial_survey.py create mode 100644 src/brainflex/core/initial_survey.py create mode 100644 tests/cli/test_initial_survey.py create mode 100644 tests/core/test_initial_survey.py diff --git a/docs/source/initial_setup.md b/docs/source/initial_setup.md index 434f53e..41e1b58 100644 --- a/docs/source/initial_setup.md +++ b/docs/source/initial_setup.md @@ -135,6 +135,14 @@ This creates a `.env` file in the repository root and copies the `config.env` fi `.env` and `config.env` are Git-ignored and should never be shared. ``` +## Initial survey + +Answer a short survey about your prior experience with Nipoppy, Neurobagel and Fed-BioMed: + +```bash +brainflex initial-survey +``` + ## Data download Download the OpenNeuro dataset assigned to your site, replacing `` with its ID (beginning with `ds`): diff --git a/src/brainflex/cli/initial_survey.py b/src/brainflex/cli/initial_survey.py new file mode 100644 index 0000000..a1ca00c --- /dev/null +++ b/src/brainflex/cli/initial_survey.py @@ -0,0 +1,38 @@ +"""CLI for ``brainflex initial-survey``.""" + +from pathlib import Path +from typing import Annotated + +import typer +from rich.prompt import Prompt + +from brainflex.cli.console import print_error +from brainflex.cli.options import RootOption +from brainflex.core.initial_survey import check_initial_survey_path, initial_survey +from brainflex.utils.constants import LIKERT_CHOICES + +TOOLS = ("Nipoppy", "Neurobagel", "Fed-BioMed") + + +def initial_survey_cli( + overwrite: Annotated[ + bool, + typer.Option("--overwrite", help="Replace existing survey answers."), + ] = False, + path: RootOption = Path(), +) -> None: + """Answer a survey on prior experience with the tools used in the experiment.""" + # check before asking questions so that answers are not lost + try: + fpath = check_initial_survey_path(path, overwrite=overwrite) + except FileExistsError as e: + print_error(e) + raise typer.Exit(1) from e + + typer.echo("Rate your familiarity (1 = not at all familiar, 5 = very familiar)") + nipoppy, neurobagel, fedbiomed = ( + int(Prompt.ask(f"How familiar are you with {tool}?", choices=LIKERT_CHOICES)) + for tool in TOOLS + ) + initial_survey(path, nipoppy, neurobagel, fedbiomed, overwrite=overwrite) + typer.echo(f"Saved answers to {fpath}") diff --git a/src/brainflex/cli/main.py b/src/brainflex/cli/main.py index e7adf0c..87d5e2c 100644 --- a/src/brainflex/cli/main.py +++ b/src/brainflex/cli/main.py @@ -8,6 +8,7 @@ from brainflex.cli.console import render_warnings from brainflex.cli.get_data import get_data_cli from brainflex.cli.init import init_cli +from brainflex.cli.initial_survey import initial_survey_cli from brainflex.cli.record import record_cli app = typer.Typer( @@ -41,4 +42,5 @@ def main( app.command("get-data")(get_data_cli) app.command("init")(init_cli) +app.command("initial-survey")(initial_survey_cli) app.command("record")(record_cli) diff --git a/src/brainflex/core/initial_survey.py b/src/brainflex/core/initial_survey.py new file mode 100644 index 0000000..aeda3e2 --- /dev/null +++ b/src/brainflex/core/initial_survey.py @@ -0,0 +1,65 @@ +"""Survey on prior experience, answered before the experiment.""" + +from datetime import datetime +from pathlib import Path + +import pandas as pd + +from brainflex.utils.constants import ( + BRAINFLEX_DIRNAME, + COL_FEDBIOMED_FAMILIARITY, + COL_NEUROBAGEL_FAMILIARITY, + COL_NIPOPPY_FAMILIARITY, + COL_TIMESTAMP, + INITIAL_SURVEY_COLUMNS, + INITIAL_SURVEY_FILENAME, + LIKERT_CHOICES, +) + + +def check_initial_survey_path(root: Path, overwrite: bool = False) -> Path: + """Return the path to the initial survey file of the project. + + Raises FileExistsError if the file already exists and ``overwrite`` is False. + """ + fpath = root / BRAINFLEX_DIRNAME / INITIAL_SURVEY_FILENAME + if fpath.exists() and not overwrite: + raise FileExistsError( + f"Initial survey already completed: {fpath} exists." + " Use --overwrite to replace it." + ) + return fpath + + +def initial_survey( + root: Path, + nipoppy: int, + neurobagel: int, + fedbiomed: int, + overwrite: bool = False, +) -> None: + """Save the answers to the initial survey. + + Raises ValueError if a rating is out of range, and FileExistsError if the survey + file already exists and ``overwrite`` is False. + """ + answers = { + COL_NIPOPPY_FAMILIARITY: nipoppy, + COL_NEUROBAGEL_FAMILIARITY: neurobagel, + COL_FEDBIOMED_FAMILIARITY: fedbiomed, + } + for col, value in answers.items(): + if str(value) not in LIKERT_CHOICES: + raise ValueError( + f"Invalid {col} {value!r}: must be one of {', '.join(LIKERT_CHOICES)}" + ) + + fpath = check_initial_survey_path(root, overwrite=overwrite) + + row = { + COL_TIMESTAMP: datetime.now().astimezone().isoformat(timespec="seconds"), + **answers, + } + pd.DataFrame([row], columns=INITIAL_SURVEY_COLUMNS).to_csv( + fpath, sep="\t", index=False + ) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index 76a4646..db331c2 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -28,6 +28,19 @@ COL_NOTES = "notes" STATUS_LOG_COLUMNS = (COL_TIMESTAMP, COL_STEP, COL_STATUS, COL_NOTES) +# initial-survey +INITIAL_SURVEY_FILENAME = "initial_survey.tsv" +COL_NIPOPPY_FAMILIARITY = "nipoppy_familiarity" +COL_NEUROBAGEL_FAMILIARITY = "neurobagel_familiarity" +COL_FEDBIOMED_FAMILIARITY = "fedbiomed_familiarity" +INITIAL_SURVEY_COLUMNS = ( + COL_TIMESTAMP, + COL_NIPOPPY_FAMILIARITY, + COL_NEUROBAGEL_FAMILIARITY, + COL_FEDBIOMED_FAMILIARITY, +) +LIKERT_CHOICES = ("1", "2", "3", "4", "5") + # get-data OPENNEURO_BUCKET = "openneuro.org" OPENNEURO_REGION = "us-east-1" diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py new file mode 100644 index 0000000..6a64a8b --- /dev/null +++ b/tests/cli/test_initial_survey.py @@ -0,0 +1,107 @@ +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from pytest_mock import MockerFixture +from typer.testing import CliRunner + +from brainflex.cli.main import app +from brainflex.utils.constants import BRAINFLEX_DIRNAME, INITIAL_SURVEY_FILENAME + + +@pytest.fixture +def mock_initial_survey(mocker: MockerFixture) -> MagicMock: + return mocker.patch("brainflex.cli.initial_survey.initial_survey") + + +@pytest.fixture +def fpath_survey(dpath_root: Path) -> Path: + return dpath_root / BRAINFLEX_DIRNAME / INITIAL_SURVEY_FILENAME + + +class TestInitialSurvey: + def test_initial_survey( + self, runner: CliRunner, mock_initial_survey: MagicMock, dpath_root: Path + ): + result = runner.invoke( + app, + ["initial-survey", "--project", str(dpath_root)], + input="1\n3\n5\n", + ) + assert result.exit_code == 0 + for tool in ("Nipoppy", "Neurobagel", "Fed-BioMed"): + assert f"How familiar are you with {tool}?" in result.stdout + assert "Saved answers to" in result.stdout + mock_initial_survey.assert_called_once_with( + dpath_root.resolve(), 1, 3, 5, overwrite=False + ) + + def test_invalid_input_reprompts( + self, runner: CliRunner, mock_initial_survey: MagicMock, dpath_root: Path + ): + result = runner.invoke( + app, + ["initial-survey", "--project", str(dpath_root)], + input="0\nabc\n2\n3\n4\n", + ) + assert result.exit_code == 0 + mock_initial_survey.assert_called_once_with( + dpath_root.resolve(), 2, 3, 4, overwrite=False + ) + + def test_default_path( + self, + runner: CliRunner, + mock_initial_survey: MagicMock, + dpath_subdir: Path, + dpath_root: Path, + monkeypatch: pytest.MonkeyPatch, + ): + monkeypatch.chdir(dpath_subdir) + result = runner.invoke(app, ["initial-survey"], input="1\n1\n1\n") + assert result.exit_code == 0 + mock_initial_survey.assert_called_once_with( + dpath_root.resolve(), 1, 1, 1, overwrite=False + ) + + def test_exists( + self, + runner: CliRunner, + mock_initial_survey: MagicMock, + dpath_root: Path, + fpath_survey: Path, + ): + fpath_survey.write_text("existing") + result = runner.invoke(app, ["initial-survey", "--project", str(dpath_root)]) + assert result.exit_code == 1 + assert "How familiar" not in result.stdout + assert "ERROR" in result.stderr + assert INITIAL_SURVEY_FILENAME in result.stderr + mock_initial_survey.assert_not_called() + + def test_exists_overwrite( + self, + runner: CliRunner, + mock_initial_survey: MagicMock, + dpath_root: Path, + fpath_survey: Path, + ): + fpath_survey.write_text("existing") + result = runner.invoke( + app, + ["initial-survey", "--overwrite", "--project", str(dpath_root)], + input="1\n1\n1\n", + ) + assert result.exit_code == 0 + mock_initial_survey.assert_called_once_with( + dpath_root.resolve(), 1, 1, 1, overwrite=True + ) + + def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Path): + result = runner.invoke( + app, + ["initial-survey", "--project", str(dpath_root)], + input="1\n3\n5\n", + ) + assert result.exit_code == 0 + assert fpath_survey.read_text().splitlines()[1].endswith("\t1\t3\t5") diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py new file mode 100644 index 0000000..6597e3f --- /dev/null +++ b/tests/core/test_initial_survey.py @@ -0,0 +1,75 @@ +from datetime import datetime +from pathlib import Path + +import pandas as pd +import pytest + +from brainflex.core.initial_survey import check_initial_survey_path, initial_survey +from brainflex.utils.constants import ( + BRAINFLEX_DIRNAME, + INITIAL_SURVEY_COLUMNS, + INITIAL_SURVEY_FILENAME, +) + + +@pytest.fixture +def root(tmp_path: Path) -> Path: + (tmp_path / BRAINFLEX_DIRNAME).mkdir() + return tmp_path + + +@pytest.fixture +def fpath_survey(root: Path) -> Path: + return root / BRAINFLEX_DIRNAME / INITIAL_SURVEY_FILENAME + + +def _read(fpath: Path) -> pd.DataFrame: + return pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) + + +class TestCheckInitialSurveyPath: + def test_not_exists(self, root: Path, fpath_survey: Path): + assert check_initial_survey_path(root) == fpath_survey + + def test_exists(self, root: Path, fpath_survey: Path): + fpath_survey.write_text("existing") + with pytest.raises(FileExistsError, match=str(fpath_survey)): + check_initial_survey_path(root) + + def test_exists_overwrite(self, root: Path, fpath_survey: Path): + fpath_survey.write_text("existing") + assert check_initial_survey_path(root, overwrite=True) == fpath_survey + + +class TestInitialSurvey: + def test_creates_file(self, root: Path, fpath_survey: Path): + initial_survey(root, 1, 3, 5) + df = _read(fpath_survey) + assert tuple(df.columns) == INITIAL_SURVEY_COLUMNS + assert len(df) == 1 + row = df.iloc[0] + assert row["nipoppy_familiarity"] == "1" + assert row["neurobagel_familiarity"] == "3" + assert row["fedbiomed_familiarity"] == "5" + assert datetime.fromisoformat(row["timestamp"]).tzinfo is not None + + def test_exists(self, root: Path, fpath_survey: Path): + fpath_survey.write_text("existing") + with pytest.raises(FileExistsError, match="--overwrite"): + initial_survey(root, 1, 1, 1) + assert fpath_survey.read_text() == "existing" + + def test_overwrite(self, root: Path, fpath_survey: Path): + initial_survey(root, 1, 1, 1) + initial_survey(root, 2, 2, 2, overwrite=True) + df = _read(fpath_survey) + assert len(df) == 1 + assert df.iloc[0]["nipoppy_familiarity"] == "2" + + @pytest.mark.parametrize("ratings", [(0, 1, 1), (1, 6, 1), (1, 1, -1)]) + def test_invalid_rating( + self, root: Path, fpath_survey: Path, ratings: tuple[int, int, int] + ): + with pytest.raises(ValueError, match="must be one of"): + initial_survey(root, *ratings) + assert not fpath_survey.exists() From cb88bbfc7c3d6b6c8dd928412b539f133c7c0541 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 16:12:40 -0500 Subject: [PATCH 02/16] remove timestamp column --- src/brainflex/core/initial_survey.py | 8 +------- src/brainflex/utils/constants.py | 1 - tests/cli/test_initial_survey.py | 2 +- tests/core/test_initial_survey.py | 2 -- 4 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/brainflex/core/initial_survey.py b/src/brainflex/core/initial_survey.py index aeda3e2..658b507 100644 --- a/src/brainflex/core/initial_survey.py +++ b/src/brainflex/core/initial_survey.py @@ -1,6 +1,5 @@ """Survey on prior experience, answered before the experiment.""" -from datetime import datetime from pathlib import Path import pandas as pd @@ -10,7 +9,6 @@ COL_FEDBIOMED_FAMILIARITY, COL_NEUROBAGEL_FAMILIARITY, COL_NIPOPPY_FAMILIARITY, - COL_TIMESTAMP, INITIAL_SURVEY_COLUMNS, INITIAL_SURVEY_FILENAME, LIKERT_CHOICES, @@ -56,10 +54,6 @@ def initial_survey( fpath = check_initial_survey_path(root, overwrite=overwrite) - row = { - COL_TIMESTAMP: datetime.now().astimezone().isoformat(timespec="seconds"), - **answers, - } - pd.DataFrame([row], columns=INITIAL_SURVEY_COLUMNS).to_csv( + pd.DataFrame([answers], columns=INITIAL_SURVEY_COLUMNS).to_csv( fpath, sep="\t", index=False ) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index db331c2..ee4381e 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -34,7 +34,6 @@ COL_NEUROBAGEL_FAMILIARITY = "neurobagel_familiarity" COL_FEDBIOMED_FAMILIARITY = "fedbiomed_familiarity" INITIAL_SURVEY_COLUMNS = ( - COL_TIMESTAMP, COL_NIPOPPY_FAMILIARITY, COL_NEUROBAGEL_FAMILIARITY, COL_FEDBIOMED_FAMILIARITY, diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py index 6a64a8b..0f06cef 100644 --- a/tests/cli/test_initial_survey.py +++ b/tests/cli/test_initial_survey.py @@ -104,4 +104,4 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Pat input="1\n3\n5\n", ) assert result.exit_code == 0 - assert fpath_survey.read_text().splitlines()[1].endswith("\t1\t3\t5") + assert fpath_survey.read_text().splitlines()[1] == "1\t3\t5" diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py index 6597e3f..0913cb0 100644 --- a/tests/core/test_initial_survey.py +++ b/tests/core/test_initial_survey.py @@ -1,4 +1,3 @@ -from datetime import datetime from pathlib import Path import pandas as pd @@ -51,7 +50,6 @@ def test_creates_file(self, root: Path, fpath_survey: Path): assert row["nipoppy_familiarity"] == "1" assert row["neurobagel_familiarity"] == "3" assert row["fedbiomed_familiarity"] == "5" - assert datetime.fromisoformat(row["timestamp"]).tzinfo is not None def test_exists(self, root: Path, fpath_survey: Path): fpath_survey.write_text("existing") From 96e9593ba330ebaa6b866646d1a8a1b97e7fc01a Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 16:33:48 -0500 Subject: [PATCH 03/16] store answers in long form format --- src/brainflex/cli/initial_survey.py | 14 +++---- src/brainflex/core/initial_survey.py | 46 ++++++++++++----------- src/brainflex/utils/constants.py | 13 +++---- tests/cli/test_initial_survey.py | 25 ++++++++++--- tests/core/test_initial_survey.py | 55 +++++++++++++++++++++------- 5 files changed, 96 insertions(+), 57 deletions(-) diff --git a/src/brainflex/cli/initial_survey.py b/src/brainflex/cli/initial_survey.py index a1ca00c..7cbfc07 100644 --- a/src/brainflex/cli/initial_survey.py +++ b/src/brainflex/cli/initial_survey.py @@ -9,9 +9,7 @@ from brainflex.cli.console import print_error from brainflex.cli.options import RootOption from brainflex.core.initial_survey import check_initial_survey_path, initial_survey -from brainflex.utils.constants import LIKERT_CHOICES - -TOOLS = ("Nipoppy", "Neurobagel", "Fed-BioMed") +from brainflex.utils.constants import INITIAL_SURVEY_QUESTIONS, LIKERT_CHOICES def initial_survey_cli( @@ -30,9 +28,9 @@ def initial_survey_cli( raise typer.Exit(1) from e typer.echo("Rate your familiarity (1 = not at all familiar, 5 = very familiar)") - nipoppy, neurobagel, fedbiomed = ( - int(Prompt.ask(f"How familiar are you with {tool}?", choices=LIKERT_CHOICES)) - for tool in TOOLS - ) - initial_survey(path, nipoppy, neurobagel, fedbiomed, overwrite=overwrite) + answers = { + question: int(Prompt.ask(question, choices=LIKERT_CHOICES)) + for question in INITIAL_SURVEY_QUESTIONS + } + initial_survey(path, answers, overwrite=overwrite) typer.echo(f"Saved answers to {fpath}") diff --git a/src/brainflex/core/initial_survey.py b/src/brainflex/core/initial_survey.py index 658b507..aec71f3 100644 --- a/src/brainflex/core/initial_survey.py +++ b/src/brainflex/core/initial_survey.py @@ -1,17 +1,18 @@ """Survey on prior experience, answered before the experiment.""" +from collections.abc import Mapping from pathlib import Path import pandas as pd from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, - COL_FEDBIOMED_FAMILIARITY, - COL_NEUROBAGEL_FAMILIARITY, - COL_NIPOPPY_FAMILIARITY, - INITIAL_SURVEY_COLUMNS, + COL_ANSWER, + COL_QUESTION, INITIAL_SURVEY_FILENAME, + INITIAL_SURVEY_QUESTIONS, LIKERT_CHOICES, + SURVEY_COLUMNS, ) @@ -30,30 +31,31 @@ def check_initial_survey_path(root: Path, overwrite: bool = False) -> Path: def initial_survey( - root: Path, - nipoppy: int, - neurobagel: int, - fedbiomed: int, - overwrite: bool = False, + root: Path, answers: Mapping[str, int], overwrite: bool = False ) -> None: """Save the answers to the initial survey. - Raises ValueError if a rating is out of range, and FileExistsError if the survey - file already exists and ``overwrite`` is False. + ``answers`` maps each question in ``INITIAL_SURVEY_QUESTIONS`` to a rating on a + Likert scale from 1 to 5. Raises ValueError if questions are missing or unknown, + or if a rating is out of range, and FileExistsError if the survey file already + exists and ``overwrite`` is False. """ - answers = { - COL_NIPOPPY_FAMILIARITY: nipoppy, - COL_NEUROBAGEL_FAMILIARITY: neurobagel, - COL_FEDBIOMED_FAMILIARITY: fedbiomed, - } - for col, value in answers.items(): - if str(value) not in LIKERT_CHOICES: + if set(answers) != set(INITIAL_SURVEY_QUESTIONS): + raise ValueError( + "Answers must be given for exactly these questions:" + f" {INITIAL_SURVEY_QUESTIONS}" + ) + for question, answer in answers.items(): + if str(answer) not in LIKERT_CHOICES: raise ValueError( - f"Invalid {col} {value!r}: must be one of {', '.join(LIKERT_CHOICES)}" + f"Invalid answer {answer!r} to {question!r}:" + f" must be one of {', '.join(LIKERT_CHOICES)}" ) fpath = check_initial_survey_path(root, overwrite=overwrite) - pd.DataFrame([answers], columns=INITIAL_SURVEY_COLUMNS).to_csv( - fpath, sep="\t", index=False - ) + rows = [ + {COL_QUESTION: question, COL_ANSWER: answers[question]} + for question in INITIAL_SURVEY_QUESTIONS + ] + pd.DataFrame(rows, columns=SURVEY_COLUMNS).to_csv(fpath, sep="\t", index=False) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index ee4381e..abc53b6 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -30,14 +30,13 @@ # initial-survey INITIAL_SURVEY_FILENAME = "initial_survey.tsv" -COL_NIPOPPY_FAMILIARITY = "nipoppy_familiarity" -COL_NEUROBAGEL_FAMILIARITY = "neurobagel_familiarity" -COL_FEDBIOMED_FAMILIARITY = "fedbiomed_familiarity" -INITIAL_SURVEY_COLUMNS = ( - COL_NIPOPPY_FAMILIARITY, - COL_NEUROBAGEL_FAMILIARITY, - COL_FEDBIOMED_FAMILIARITY, +INITIAL_SURVEY_QUESTIONS = tuple( + f"How familiar are you with {tool}?" + for tool in ("Nipoppy", "Neurobagel", "Fed-BioMed") ) +COL_QUESTION = "question" +COL_ANSWER = "answer" +SURVEY_COLUMNS = (COL_QUESTION, COL_ANSWER) LIKERT_CHOICES = ("1", "2", "3", "4", "5") # get-data diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py index 0f06cef..ac7acbb 100644 --- a/tests/cli/test_initial_survey.py +++ b/tests/cli/test_initial_survey.py @@ -6,7 +6,11 @@ from typer.testing import CliRunner from brainflex.cli.main import app -from brainflex.utils.constants import BRAINFLEX_DIRNAME, INITIAL_SURVEY_FILENAME +from brainflex.utils.constants import ( + BRAINFLEX_DIRNAME, + INITIAL_SURVEY_FILENAME, + INITIAL_SURVEY_QUESTIONS, +) @pytest.fixture @@ -14,6 +18,10 @@ def mock_initial_survey(mocker: MockerFixture) -> MagicMock: return mocker.patch("brainflex.cli.initial_survey.initial_survey") +def _answers(*ratings: int) -> dict[str, int]: + return dict(zip(INITIAL_SURVEY_QUESTIONS, ratings, strict=True)) + + @pytest.fixture def fpath_survey(dpath_root: Path) -> Path: return dpath_root / BRAINFLEX_DIRNAME / INITIAL_SURVEY_FILENAME @@ -33,7 +41,7 @@ def test_initial_survey( assert f"How familiar are you with {tool}?" in result.stdout assert "Saved answers to" in result.stdout mock_initial_survey.assert_called_once_with( - dpath_root.resolve(), 1, 3, 5, overwrite=False + dpath_root.resolve(), _answers(1, 3, 5), overwrite=False ) def test_invalid_input_reprompts( @@ -46,7 +54,7 @@ def test_invalid_input_reprompts( ) assert result.exit_code == 0 mock_initial_survey.assert_called_once_with( - dpath_root.resolve(), 2, 3, 4, overwrite=False + dpath_root.resolve(), _answers(2, 3, 4), overwrite=False ) def test_default_path( @@ -61,7 +69,7 @@ def test_default_path( result = runner.invoke(app, ["initial-survey"], input="1\n1\n1\n") assert result.exit_code == 0 mock_initial_survey.assert_called_once_with( - dpath_root.resolve(), 1, 1, 1, overwrite=False + dpath_root.resolve(), _answers(1, 1, 1), overwrite=False ) def test_exists( @@ -94,7 +102,7 @@ def test_exists_overwrite( ) assert result.exit_code == 0 mock_initial_survey.assert_called_once_with( - dpath_root.resolve(), 1, 1, 1, overwrite=True + dpath_root.resolve(), _answers(1, 1, 1), overwrite=True ) def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Path): @@ -104,4 +112,9 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Pat input="1\n3\n5\n", ) assert result.exit_code == 0 - assert fpath_survey.read_text().splitlines()[1] == "1\t3\t5" + assert fpath_survey.read_text().splitlines() == [ + "question\tanswer", + "How familiar are you with Nipoppy?\t1", + "How familiar are you with Neurobagel?\t3", + "How familiar are you with Fed-BioMed?\t5", + ] diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py index 0913cb0..eaa98c9 100644 --- a/tests/core/test_initial_survey.py +++ b/tests/core/test_initial_survey.py @@ -6,8 +6,9 @@ from brainflex.core.initial_survey import check_initial_survey_path, initial_survey from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, - INITIAL_SURVEY_COLUMNS, INITIAL_SURVEY_FILENAME, + INITIAL_SURVEY_QUESTIONS, + SURVEY_COLUMNS, ) @@ -26,6 +27,10 @@ def _read(fpath: Path) -> pd.DataFrame: return pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) +def _answers(*ratings: int) -> dict[str, int]: + return dict(zip(INITIAL_SURVEY_QUESTIONS, ratings, strict=True)) + + class TestCheckInitialSurveyPath: def test_not_exists(self, root: Path, fpath_survey: Path): assert check_initial_survey_path(root) == fpath_survey @@ -42,32 +47,54 @@ def test_exists_overwrite(self, root: Path, fpath_survey: Path): class TestInitialSurvey: def test_creates_file(self, root: Path, fpath_survey: Path): - initial_survey(root, 1, 3, 5) + initial_survey(root, _answers(1, 3, 5)) + df = _read(fpath_survey) + assert tuple(df.columns) == SURVEY_COLUMNS + assert list(df["question"]) == [ + "How familiar are you with Nipoppy?", + "How familiar are you with Neurobagel?", + "How familiar are you with Fed-BioMed?", + ] + assert list(df["answer"]) == ["1", "3", "5"] + + def test_question_order(self, root: Path, fpath_survey: Path): + answers = dict(reversed(_answers(1, 3, 5).items())) + initial_survey(root, answers) df = _read(fpath_survey) - assert tuple(df.columns) == INITIAL_SURVEY_COLUMNS - assert len(df) == 1 - row = df.iloc[0] - assert row["nipoppy_familiarity"] == "1" - assert row["neurobagel_familiarity"] == "3" - assert row["fedbiomed_familiarity"] == "5" + assert list(df["question"]) == list(INITIAL_SURVEY_QUESTIONS) + assert list(df["answer"]) == ["1", "3", "5"] def test_exists(self, root: Path, fpath_survey: Path): fpath_survey.write_text("existing") with pytest.raises(FileExistsError, match="--overwrite"): - initial_survey(root, 1, 1, 1) + initial_survey(root, _answers(1, 1, 1)) assert fpath_survey.read_text() == "existing" def test_overwrite(self, root: Path, fpath_survey: Path): - initial_survey(root, 1, 1, 1) - initial_survey(root, 2, 2, 2, overwrite=True) + initial_survey(root, _answers(1, 1, 1)) + initial_survey(root, _answers(2, 2, 2), overwrite=True) df = _read(fpath_survey) - assert len(df) == 1 - assert df.iloc[0]["nipoppy_familiarity"] == "2" + assert len(df) == len(INITIAL_SURVEY_QUESTIONS) + assert list(df["answer"]) == ["2", "2", "2"] @pytest.mark.parametrize("ratings", [(0, 1, 1), (1, 6, 1), (1, 1, -1)]) def test_invalid_rating( self, root: Path, fpath_survey: Path, ratings: tuple[int, int, int] ): with pytest.raises(ValueError, match="must be one of"): - initial_survey(root, *ratings) + initial_survey(root, _answers(*ratings)) + assert not fpath_survey.exists() + + @pytest.mark.parametrize( + "answers", + [ + dict(list(_answers(1, 1, 1).items())[:2]), + {**_answers(1, 1, 1), "Unknown question?": 1}, + ], + ) + def test_invalid_questions( + self, root: Path, fpath_survey: Path, answers: dict[str, int] + ): + with pytest.raises(ValueError, match="exactly these questions"): + initial_survey(root, answers) assert not fpath_survey.exists() From 3cdfc050d884686d7ae9df218d323addc93824d4 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 17:13:52 -0500 Subject: [PATCH 04/16] update record command --- src/brainflex/cli/record.py | 26 ++-- src/brainflex/core/record.py | 128 ++++++++++++------ src/brainflex/utils/constants.py | 33 +++-- src/brainflex/utils/tsv.py | 49 +++++++ tests/cli/test_options.py | 3 +- tests/cli/test_record.py | 114 +++++++++++++--- tests/core/test_record.py | 224 ++++++++++++++++++++++--------- tests/utils/test_tsv.py | 68 ++++++++++ 8 files changed, 503 insertions(+), 142 deletions(-) create mode 100644 src/brainflex/utils/tsv.py create mode 100644 tests/utils/test_tsv.py diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index df856f8..905f2c0 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -7,7 +7,10 @@ from rich.prompt import Prompt from brainflex.cli.options import RootOption -from brainflex.core.record import Status, Step, record +from brainflex.core.record import Status, Step, get_step_questions, record +from brainflex.utils.constants import QUESTION_TOOLS_EASIER + +HINTS = {QUESTION_TOOLS_EASIER: "1 = not at all, 5 = a lot"} def record_cli( @@ -16,12 +19,19 @@ def record_cli( path: RootOption = Path(), ) -> None: """Record the start or end of an experiment step.""" - notes = "" + answers = {} if status == Status.END: - notes = Prompt.ask( - "Notes/feedback (press Enter to leave blank)", - default=notes, - show_default=False, - ) - record(path, step, status, notes=notes) + for question, choices in get_step_questions(step).items(): + if choices is None: + answers[question] = Prompt.ask( + f"{question} (press Enter to leave blank)", + default="", + show_default=False, + ) + else: + hint = f" ({HINTS[question]})" if question in HINTS else "" + answers[question] = Prompt.ask( + f"{question}{hint}", choices=list(choices) + ) + record(path, step, status, answers=answers) typer.echo(f"Recorded {step} {status}") diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 7b3e384..bc5e78e 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -1,28 +1,39 @@ """Record the start and end of experiment steps.""" import warnings +from collections.abc import Mapping from datetime import datetime from enum import StrEnum from pathlib import Path -import pandas as pd - from brainflex.utils.constants import ( - BACKUP_TIMESTAMP_FORMAT, BRAINFLEX_DIRNAME, - COL_NOTES, + COL_ANSWER, + COL_QUESTION, COL_STATUS, COL_STEP, COL_TIMESTAMP, - STATUS_LOG_COLUMNS, - STATUS_LOG_FILENAME, + LIKERT_CHOICES, + QUESTION_NOTES, + QUESTION_TIME_WITHOUT_TOOLS, + QUESTION_TOOLS_EASIER, + STEP_SURVEY_COLUMNS, + STEP_SURVEY_FILENAME, + TIME_WITHOUT_TOOLS_CHOICES, + TIMESTAMPS_COLUMNS, + TIMESTAMPS_FILENAME, ) +from brainflex.utils.tsv import append_tsv, load_tsv class Step(StrEnum): """Experiment steps.""" - GET_DATA = "get-data" + IDP_EXTRACTION = "idp_extraction" + SEMANTIC_HARMONIZATION = "semantic_harmonization" + FL_READY_DATA = "fl_ready_data" + FEDBIOMED_NODE_PART_1 = "fedbiomed_node_part_1" + FEDBIOMED_NODE_PART_2 = "fedbiomed_node_part_2" class Status(StrEnum): @@ -32,40 +43,61 @@ class Status(StrEnum): END = "end" -def _load_status_log(fpath: Path, now: datetime) -> pd.DataFrame: - """Load the status log, or return an empty one if it does not exist. - - If the file cannot be parsed or is missing expected columns, it is moved to a - timestamped backup and an empty log is returned. - """ - if not fpath.exists(): - return pd.DataFrame(columns=STATUS_LOG_COLUMNS) - - try: - df = pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) - except (pd.errors.EmptyDataError, pd.errors.ParserError): - df = None - - if df is not None and set(STATUS_LOG_COLUMNS).issubset(df.columns): - return df - - fpath_backup = fpath.with_name( - f"{fpath.stem}-{now.strftime(BACKUP_TIMESTAMP_FORMAT)}{fpath.suffix}" - ) - fpath.rename(fpath_backup) - warnings.warn(f"invalid status log moved to {fpath_backup}", stacklevel=3) - return pd.DataFrame(columns=STATUS_LOG_COLUMNS) - - -def record(root: Path, step: Step, status: Status, notes: str = "") -> None: - """Add a timestamped row to the status log, creating it if needed. - - Warns if a step is started twice or ended without being started. - """ - now = datetime.now().astimezone() - df = _load_status_log(fpath := root / BRAINFLEX_DIRNAME / STATUS_LOG_FILENAME, now) +DATA_PREP_STEPS = frozenset( + {Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATA} +) - statuses = df.query(f"{COL_STEP} == @step")[COL_STATUS] +# 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: LIKERT_CHOICES, + QUESTION_TIME_WITHOUT_TOOLS: TIME_WITHOUT_TOOLS_CHOICES, +} +COMMON_QUESTIONS: dict[str, tuple[str, ...] | None] = {QUESTION_NOTES: None} + + +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.""" + if step in DATA_PREP_STEPS: + return DATA_PREP_QUESTIONS | COMMON_QUESTIONS + return dict(COMMON_QUESTIONS) + + +def _check_answers( + step: Step, status: Status, answers: Mapping[str, str] +) -> dict[str, tuple[str, ...] | None]: + """Raise ValueError if the answers don't match the questions for the step.""" + questions = get_step_questions(step) if status == Status.END else {} + if set(answers) != set(questions): + raise ValueError( + f"Answers for {step} {status} must be given for exactly these" + f" questions: {list(questions)}" + ) + for question, answer in answers.items(): + choices = questions[question] + if choices is not None and answer not in choices: + raise ValueError( + f"Invalid answer {answer!r} to {question!r}:" + f" must be one of {', '.join(choices)}" + ) + return questions + + +def record( + root: Path, + step: Step, + status: Status, + answers: Mapping[str, str] | None = None, +) -> None: + """Record the start or end of a step + some survey answers.""" + answers = answers or {} + questions = _check_answers(step, status, answers) + + dpath = root / BRAINFLEX_DIRNAME + fpath_timestamps = dpath / TIMESTAMPS_FILENAME + df_timestamps = load_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS) + + statuses = df_timestamps.query(f"{COL_STEP} == @step")[COL_STATUS] last_status = statuses.iloc[-1] if not statuses.empty else None if status == Status.START and last_status == Status.START: warnings.warn(f"step {step} was already started", stacklevel=2) @@ -73,10 +105,18 @@ def record(root: Path, step: Step, status: Status, notes: str = "") -> None: warnings.warn(f"step {step} was not started", stacklevel=2) row = { - COL_TIMESTAMP: now.isoformat(timespec="seconds"), + COL_TIMESTAMP: datetime.now().astimezone().isoformat(timespec="seconds"), COL_STEP: str(step), COL_STATUS: str(status), - COL_NOTES: notes, } - df = pd.concat([df, pd.DataFrame([row], columns=STATUS_LOG_COLUMNS)]) - df.to_csv(fpath, sep="\t", index=False) + append_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS, [row], df=df_timestamps) + + if questions: + append_tsv( + dpath / STEP_SURVEY_FILENAME, + STEP_SURVEY_COLUMNS, + ( + {COL_STEP: str(step), COL_QUESTION: q, COL_ANSWER: answers[q]} + for q in questions + ), + ) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index abc53b6..5a6814b 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -19,14 +19,12 @@ VPN_CONFIG_FILENAME = "config.env" VPN_CONFIG_VAR_NODE_ID = "FEDBIOMED_NODEID" -# record -STATUS_LOG_FILENAME = "status_log.tsv" +# surveys +COL_QUESTION = "question" +COL_ANSWER = "answer" +SURVEY_COLUMNS = (COL_QUESTION, COL_ANSWER) +LIKERT_CHOICES = ("1", "2", "3", "4", "5") BACKUP_TIMESTAMP_FORMAT = "%Y%m%dT%H%M%S" -COL_TIMESTAMP = "timestamp" -COL_STEP = "step" -COL_STATUS = "status" -COL_NOTES = "notes" -STATUS_LOG_COLUMNS = (COL_TIMESTAMP, COL_STEP, COL_STATUS, COL_NOTES) # initial-survey INITIAL_SURVEY_FILENAME = "initial_survey.tsv" @@ -34,10 +32,23 @@ f"How familiar are you with {tool}?" for tool in ("Nipoppy", "Neurobagel", "Fed-BioMed") ) -COL_QUESTION = "question" -COL_ANSWER = "answer" -SURVEY_COLUMNS = (COL_QUESTION, COL_ANSWER) -LIKERT_CHOICES = ("1", "2", "3", "4", "5") + +# record +TIMESTAMPS_FILENAME = "timestamps.tsv" +COL_TIMESTAMP = "timestamp" +COL_STEP = "step" +COL_STATUS = "status" +TIMESTAMPS_COLUMNS = (COL_TIMESTAMP, COL_STEP, COL_STATUS) +STEP_SURVEY_FILENAME = "step_survey.tsv" +STEP_SURVEY_COLUMNS = (COL_STEP, *SURVEY_COLUMNS) +QUESTION_TOOLS_EASIER = ( + "By how much did the Nipoppy/Neurobagel tools make things easier?" +) +QUESTION_TIME_WITHOUT_TOOLS = ( + "How long do you think this would have taken without the Nipoppy/Neurobagel tools?" +) +TIME_WITHOUT_TOOLS_CHOICES = ("<1h", "1-4h", "4-8h", "1-3d", "3-7d", ">7d") +QUESTION_NOTES = "Notes/feedback" # get-data OPENNEURO_BUCKET = "openneuro.org" diff --git a/src/brainflex/utils/tsv.py b/src/brainflex/utils/tsv.py new file mode 100644 index 0000000..7ba20f6 --- /dev/null +++ b/src/brainflex/utils/tsv.py @@ -0,0 +1,49 @@ +"""TSV file utilities.""" + +import warnings +from collections.abc import Iterable, Mapping, Sequence +from datetime import datetime +from pathlib import Path + +import pandas as pd + +from brainflex.utils.constants import BACKUP_TIMESTAMP_FORMAT + + +def load_tsv(fpath: Path, columns: Sequence[str]) -> pd.DataFrame: + """Load a TSV file, or return an empty one if it does not exist. + + If the file cannot be parsed or is missing expected columns, it is moved to a + timestamped backup and an empty dataframe is returned. + """ + if not fpath.exists(): + return pd.DataFrame(columns=columns) + + try: + df = pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) + except (pd.errors.EmptyDataError, pd.errors.ParserError): + df = None + + if df is not None and set(columns).issubset(df.columns): + return df + + now = datetime.now().astimezone() + fpath_backup = fpath.with_name( + f"{fpath.stem}-{now.strftime(BACKUP_TIMESTAMP_FORMAT)}{fpath.suffix}" + ) + fpath.rename(fpath_backup) + warnings.warn(f"invalid file {fpath} moved to {fpath_backup}", stacklevel=3) + return pd.DataFrame(columns=columns) + + +def append_tsv( + fpath: Path, + columns: Sequence[str], + rows: Iterable[Mapping[str, str]], + df: pd.DataFrame | None = None, +) -> None: + """Append rows to a TSV file, creating it if needed.""" + if df is None: + df = load_tsv(fpath, columns) + df = pd.concat([df, pd.DataFrame(list(rows), columns=columns)]) + df.to_csv(fpath, sep="\t", index=False) diff --git a/tests/cli/test_options.py b/tests/cli/test_options.py index 09fa4fc..91d270d 100644 --- a/tests/cli/test_options.py +++ b/tests/cli/test_options.py @@ -13,7 +13,8 @@ class TestRootOption: [ (["get-data", "ds000001"], "brainflex.cli.get_data.get_data"), (["init", "config.env"], "brainflex.cli.init.init"), - (["record", "get-data", "start"], "brainflex.cli.record.record"), + (["initial-survey"], "brainflex.cli.initial_survey.initial_survey"), + (["record", "idp_extraction", "start"], "brainflex.cli.record.record"), ], ) @pytest.mark.parametrize("explicit", [True, False]) diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index b55187d..4cb86a2 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -2,12 +2,21 @@ from pathlib import Path from unittest.mock import MagicMock +import pandas as pd import pytest from pytest_mock import MockerFixture from typer.testing import CliRunner from brainflex.cli.main import app from brainflex.core.record import Status, Step +from brainflex.utils.constants import ( + BRAINFLEX_DIRNAME, + QUESTION_NOTES, + QUESTION_TIME_WITHOUT_TOOLS, + QUESTION_TOOLS_EASIER, + STEP_SURVEY_FILENAME, + TIMESTAMPS_FILENAME, +) @pytest.fixture @@ -18,19 +27,19 @@ def mock_record(mocker: MockerFixture) -> MagicMock: class TestRecord: def test_start(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path): result = runner.invoke( - app, ["record", "get-data", "start", "--project", str(dpath_root)] + app, ["record", "idp_extraction", "start", "--project", str(dpath_root)] ) assert result.exit_code == 0 - assert "Notes" not in result.stdout - assert "Recorded get-data start" in result.stdout + assert "?" not in result.stdout + assert "Recorded idp_extraction start" in result.stdout mock_record.assert_called_once_with( - dpath_root.resolve(), Step.GET_DATA, Status.START, notes="" + dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} ) @pytest.mark.parametrize( ("user_input", "notes"), [("went fine\n", "went fine"), ("\n", "")] ) - def test_end( + def test_end_other( self, runner: CliRunner, mock_record: MagicMock, @@ -40,13 +49,68 @@ def test_end( ): result = runner.invoke( app, - ["record", "get-data", "end", "--project", str(dpath_root)], + ["record", "fedbiomed_node_part_1", "end", "--project", str(dpath_root)], input=user_input, ) assert result.exit_code == 0 - assert "Notes" in result.stdout + assert QUESTION_NOTES in result.stdout + assert QUESTION_TOOLS_EASIER not in result.stdout + assert QUESTION_TIME_WITHOUT_TOOLS not in result.stdout mock_record.assert_called_once_with( - dpath_root.resolve(), Step.GET_DATA, Status.END, notes=notes + dpath_root.resolve(), + Step.FEDBIOMED_NODE_PART_1, + Status.END, + answers={QUESTION_NOTES: notes}, + ) + + @pytest.mark.parametrize( + "step", [Step.IDP_EXTRACTION, Step.SEMANTIC_HARMONIZATION, Step.FL_READY_DATA] + ) + def test_end_data_prep( + self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path, step: Step + ): + result = runner.invoke( + app, + ["record", str(step), "end", "--project", str(dpath_root)], + input="4\n1-4h\nwent fine\n", + ) + assert result.exit_code == 0 + stdout = 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(QUESTION_NOTES) + ) + mock_record.assert_called_once_with( + dpath_root.resolve(), + step, + Status.END, + answers={ + QUESTION_TOOLS_EASIER: "4", + QUESTION_TIME_WITHOUT_TOOLS: "1-4h", + QUESTION_NOTES: "went fine", + }, + ) + + def test_end_data_prep_invalid_input_reprompts( + self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path + ): + result = runner.invoke( + app, + ["record", "idp_extraction", "end", "--project", str(dpath_root)], + input="6\n5\n2 days\n>7d\n\n", + ) + assert result.exit_code == 0 + mock_record.assert_called_once_with( + dpath_root.resolve(), + Step.IDP_EXTRACTION, + Status.END, + answers={ + QUESTION_TOOLS_EASIER: "5", + QUESTION_TIME_WITHOUT_TOOLS: ">7d", + QUESTION_NOTES: "", + }, ) def test_default_path( @@ -57,10 +121,10 @@ def test_default_path( monkeypatch: pytest.MonkeyPatch, ): monkeypatch.chdir(dpath_root) - result = runner.invoke(app, ["record", "get-data", "start"]) + result = runner.invoke(app, ["record", "idp_extraction", "start"]) assert result.exit_code == 0 mock_record.assert_called_once_with( - dpath_root.resolve(), Step.GET_DATA, Status.START, notes="" + dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} ) @pytest.mark.parametrize("explicit", [True, False]) @@ -78,18 +142,19 @@ def test_subdirectory( app, [ "record", - "get-data", + "idp_extraction", "start", *(["--project", str(dpath_subdir)] if explicit else []), ], ) assert result.exit_code == 0 mock_record.assert_called_once_with( - dpath_root.resolve(), Step.GET_DATA, Status.START, notes="" + dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} ) @pytest.mark.parametrize( - "args", [["bad-step", "start"], ["get-data", "bad-status"]] + "args", + [["get-data", "start"], ["bad-step", "start"], ["idp_extraction", "bad"]], ) def test_invalid_choice( self, runner: CliRunner, mock_record: MagicMock, args: list[str] @@ -100,10 +165,27 @@ def test_invalid_choice( def test_warning(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path): mock_record.side_effect = lambda *args, **kwargs: warnings.warn( - "step get-data was already started", stacklevel=2 + "step idp_extraction was already started", stacklevel=2 ) result = runner.invoke( - app, ["record", "get-data", "start", "--project", str(dpath_root)] + app, ["record", "idp_extraction", "start", "--project", str(dpath_root)] ) assert result.exit_code == 0 - assert "WARNING: step get-data was already started" in result.stderr + assert "WARNING: step idp_extraction was already started" in result.stderr + + def test_end_to_end(self, runner: CliRunner, dpath_root: Path): + args = ["record", "fl_ready_data", "--project", str(dpath_root)] + args_start = [*args[:2], "start", *args[2:]] + args_end = [*args[:2], "end", *args[2:]] + assert runner.invoke(app, args_start).exit_code == 0 + assert runner.invoke(app, args_end, input="3\n<1h\nok\n").exit_code == 0 + dpath = dpath_root / BRAINFLEX_DIRNAME + df_timestamps = pd.read_csv(dpath / TIMESTAMPS_FILENAME, sep="\t", dtype=str) + assert list(df_timestamps.columns) == ["timestamp", "step", "status"] + 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}\t3", + f"fl_ready_data\t{QUESTION_TIME_WITHOUT_TOOLS}\t<1h", + f"fl_ready_data\t{QUESTION_NOTES}\tok", + ] diff --git a/tests/core/test_record.py b/tests/core/test_record.py index cda572c..b54fcd2 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -5,13 +5,30 @@ import pandas as pd import pytest -from brainflex.core.record import Status, Step, record +from brainflex.core.record import ( + DATA_PREP_STEPS, + Status, + Step, + get_step_questions, + record, +) from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, - STATUS_LOG_COLUMNS, - STATUS_LOG_FILENAME, + 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: "4", + QUESTION_TIME_WITHOUT_TOOLS: "1-4h", + QUESTION_NOTES: "went fine", +} + @pytest.fixture def root(tmp_path: Path) -> Path: @@ -20,45 +37,149 @@ def root(tmp_path: Path) -> Path: @pytest.fixture -def fpath_log(root: Path) -> Path: - return root / BRAINFLEX_DIRNAME / STATUS_LOG_FILENAME +def fpath_timestamps(root: Path) -> Path: + return root / BRAINFLEX_DIRNAME / TIMESTAMPS_FILENAME + + +@pytest.fixture +def fpath_survey(root: Path) -> Path: + return root / BRAINFLEX_DIRNAME / STEP_SURVEY_FILENAME def _read(fpath: Path) -> pd.DataFrame: return pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) +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, + QUESTION_NOTES, + ] + + @pytest.mark.parametrize("step", sorted(set(Step) - DATA_PREP_STEPS)) + def test_other(self, step: Step): + assert list(get_step_questions(step)) == [QUESTION_NOTES] + + class TestRecord: - def test_creates_file(self, root: Path, fpath_log: Path): - record(root, Step.GET_DATA, Status.START) - df = _read(fpath_log) - assert tuple(df.columns) == STATUS_LOG_COLUMNS + def test_start(self, root: Path, fpath_timestamps: Path, fpath_survey: Path): + record(root, Step.IDP_EXTRACTION, Status.START) + df = _read(fpath_timestamps) + assert tuple(df.columns) == TIMESTAMPS_COLUMNS assert len(df) == 1 row = df.iloc[0] - assert row["step"] == "get-data" + assert row["step"] == "idp_extraction" assert row["status"] == "start" - assert row["notes"] == "" assert datetime.fromisoformat(row["timestamp"]).tzinfo is not None + assert not fpath_survey.exists() - def test_appends(self, root: Path, fpath_log: Path): - record(root, Step.GET_DATA, Status.START) - record(root, Step.GET_DATA, Status.END, notes="went fine") - df = _read(fpath_log) - assert list(df["status"]) == ["start", "end"] - assert list(df["notes"]) == ["", "went fine"] - assert fpath_log.read_text().count("timestamp") == 1 + def test_end_data_prep( + self, root: Path, fpath_timestamps: Path, fpath_survey: Path + ): + record(root, Step.IDP_EXTRACTION, Status.START) + record(root, Step.IDP_EXTRACTION, Status.END, answers=DATA_PREP_ANSWERS) + assert list(_read(fpath_timestamps)["status"]) == ["start", "end"] + assert fpath_timestamps.read_text().count("timestamp") == 1 + df = _read(fpath_survey) + assert tuple(df.columns) == STEP_SURVEY_COLUMNS + assert df.to_dict("records") == [ + {"step": "idp_extraction", "question": q, "answer": a} + for q, a in DATA_PREP_ANSWERS.items() + ] + + def test_end_other(self, root: Path, fpath_survey: Path): + record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) + record( + root, Step.FEDBIOMED_NODE_PART_1, Status.END, answers={QUESTION_NOTES: ""} + ) + assert _read(fpath_survey).to_dict("records") == [ + {"step": "fedbiomed_node_part_1", "question": QUESTION_NOTES, "answer": ""} + ] + + 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_DATA, + Status.END, + answers=dict(reversed(DATA_PREP_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 fpath_survey.read_text().count("question") == 1 - def test_notes_with_special_characters(self, root: Path, fpath_log: Path): + def test_notes_with_special_characters(self, root: Path, fpath_survey: Path): notes = 'tab\there\nnew line and "quotes"' - record(root, Step.GET_DATA, Status.START) - record(root, Step.GET_DATA, Status.END, notes=notes) - assert _read(fpath_log).iloc[-1]["notes"] == notes + record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) + record( + root, + Step.FEDBIOMED_NODE_PART_1, + Status.END, + answers={QUESTION_NOTES: notes}, + ) + assert _read(fpath_survey).iloc[-1]["answer"] == notes + + @pytest.mark.parametrize( + ("step", "status", "answers"), + [ + (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, {"Unknown?": ""}), + ], + ) + def test_invalid_questions( + self, + root: Path, + fpath_timestamps: Path, + fpath_survey: Path, + step: Step, + status: Status, + answers: dict[str, str] | None, + ): + with pytest.raises(ValueError, match="exactly these questions"): + record(root, step, status, answers=answers) + assert not fpath_timestamps.exists() + assert not fpath_survey.exists() + + @pytest.mark.parametrize( + ("question", "answer"), + [ + (QUESTION_TOOLS_EASIER, "0"), + (QUESTION_TOOLS_EASIER, "6"), + (QUESTION_TIME_WITHOUT_TOOLS, "2 days"), + ], + ) + def test_invalid_answer( + self, root: Path, fpath_timestamps: Path, question: str, answer: str + ): + with pytest.raises(ValueError, match="must be one of"): + record( + root, + Step.SEMANTIC_HARMONIZATION, + Status.END, + answers={**DATA_PREP_ANSWERS, question: answer}, + ) + assert not fpath_timestamps.exists() def test_no_warning(self, root: Path): with warnings.catch_warnings(): warnings.simplefilter("error") - record(root, Step.GET_DATA, Status.START) - record(root, Step.GET_DATA, Status.END) + record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) + record( + root, + Step.FEDBIOMED_NODE_PART_1, + Status.END, + answers={QUESTION_NOTES: ""}, + ) @pytest.mark.parametrize( ("statuses", "message"), @@ -71,48 +192,27 @@ def test_no_warning(self, root: Path): def test_warning( self, root: Path, - fpath_log: Path, + fpath_timestamps: Path, statuses: list[Status], message: str, ): - for status in statuses[:-1]: - record(root, Step.GET_DATA, status) - with pytest.warns(UserWarning, match=message): - record(root, Step.GET_DATA, statuses[-1]) - assert len(_read(fpath_log)) == len(statuses) + step = Step.FEDBIOMED_NODE_PART_1 - def test_extra_columns_kept(self, root: Path, fpath_log: Path): - fpath_log.write_text( - "timestamp\tstep\tstatus\tnotes\textra\nt0\tget-data\tstart\t\tx\n" - ) - record(root, Step.GET_DATA, Status.END) - df = _read(fpath_log) - assert list(df["status"]) == ["start", "end"] - assert list(df["extra"]) == ["x", ""] + def _record(status: Status) -> None: + answers = {QUESTION_NOTES: ""} if status == Status.END else None + record(root, step, status, answers=answers) - @pytest.mark.parametrize( - "content", - [ - "", - "timestamp\tstep\tstatus\nt0\tget-data\tstart\n", - 'timestamp\tstep\tstatus\tnotes\n"unclosed\n', - ], - ) - def test_invalid_log( - self, - root: Path, - fpath_log: Path, - content: str, - ): - fpath_log.write_text(content) - with pytest.warns(UserWarning, match="invalid status log") as record_warnings: - record(root, Step.GET_DATA, Status.START) - - fpaths_backup = list(fpath_log.parent.glob("status_log-*.tsv")) - assert len(fpaths_backup) == 1 - assert fpaths_backup[0].read_text() == content - df = _read(fpath_log) - assert tuple(df.columns) == STATUS_LOG_COLUMNS - assert len(df) == 1 + for status in statuses[:-1]: + _record(status) + with pytest.warns(UserWarning, match=message): + _record(statuses[-1]) + assert len(_read(fpath_timestamps)) == len(statuses) + + def test_invalid_timestamps_file(self, root: Path, fpath_timestamps: Path): + fpath_timestamps.write_text("timestamp\tstep\nt0\tidp_extraction\n") + with pytest.warns(UserWarning, match="invalid file") as record_warnings: + record(root, Step.IDP_EXTRACTION, Status.START) + assert len(list(fpath_timestamps.parent.glob("timestamps-*.tsv"))) == 1 + assert len(_read(fpath_timestamps)) == 1 # previous records are discarded, so no start/end warning assert len(record_warnings) == 1 diff --git a/tests/utils/test_tsv.py b/tests/utils/test_tsv.py new file mode 100644 index 0000000..af68751 --- /dev/null +++ b/tests/utils/test_tsv.py @@ -0,0 +1,68 @@ +from pathlib import Path + +import pandas as pd +import pytest + +from brainflex.utils.tsv import append_tsv, load_tsv + +COLUMNS = ("a", "b") + + +@pytest.fixture +def fpath(tmp_path: Path) -> Path: + return tmp_path / "file.tsv" + + +def _read(fpath: Path) -> pd.DataFrame: + return pd.read_csv(fpath, sep="\t", dtype=str, keep_default_na=False) + + +class TestLoadTsv: + def test_missing(self, fpath: Path): + df = load_tsv(fpath, COLUMNS) + assert tuple(df.columns) == COLUMNS + assert df.empty + assert not fpath.exists() + + def test_valid(self, fpath: Path): + fpath.write_text("a\tb\textra\n1\t2\tx\n") + df = load_tsv(fpath, COLUMNS) + assert df.to_dict("records") == [{"a": "1", "b": "2", "extra": "x"}] + + @pytest.mark.parametrize("content", ["", "a\n1\n", 'a\tb\n"unclosed\n']) + def test_invalid(self, fpath: Path, content: str): + fpath.write_text(content) + with pytest.warns(UserWarning, match="invalid file"): + df = load_tsv(fpath, COLUMNS) + assert tuple(df.columns) == COLUMNS + assert df.empty + assert not fpath.exists() + fpaths_backup = list(fpath.parent.glob("file-*.tsv")) + assert len(fpaths_backup) == 1 + assert fpaths_backup[0].read_text() == content + + +class TestAppendTsv: + def test_creates_file(self, fpath: Path): + append_tsv(fpath, COLUMNS, [{"a": "1", "b": "2"}]) + assert fpath.read_text() == "a\tb\n1\t2\n" + + def test_appends(self, fpath: Path): + append_tsv(fpath, COLUMNS, [{"a": "1", "b": "2"}]) + append_tsv(fpath, COLUMNS, ({"a": str(i), "b": ""} for i in range(2))) + assert list(_read(fpath)["a"]) == ["1", "0", "1"] + + def test_extra_columns_kept(self, fpath: Path): + fpath.write_text("a\tb\textra\n1\t2\tx\n") + append_tsv(fpath, COLUMNS, [{"a": "3", "b": "4"}]) + assert list(_read(fpath)["extra"]) == ["x", ""] + + def test_special_characters(self, fpath: Path): + value = 'tab\there\nnew line and "quotes"' + append_tsv(fpath, COLUMNS, [{"a": value, "b": ""}]) + assert _read(fpath).iloc[0]["a"] == value + + def test_given_df(self, fpath: Path): + df = pd.DataFrame([{"a": "0", "b": "0"}], columns=COLUMNS) + append_tsv(fpath, COLUMNS, [{"a": "1", "b": "2"}], df=df) + assert list(_read(fpath)["a"]) == ["0", "1"] From 73d726d5d17da20f2e5177580469ec693f3c28f4 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 17:34:13 -0500 Subject: [PATCH 05/16] update docs --- docs/source/fedbiomed_node_part_1.md | 17 ++++++++++++++++- docs/source/fedbiomed_node_part_2.md | 12 ++++++++++++ docs/source/fl_ready_dataset.md | 12 ++++++++++++ docs/source/freesurfer_idps.md | 12 ++++++++++++ docs/source/phenotypic_files.md | 12 ++++++++++++ 5 files changed, 64 insertions(+), 1 deletion(-) diff --git a/docs/source/fedbiomed_node_part_1.md b/docs/source/fedbiomed_node_part_1.md index f52332c..22f4479 100644 --- a/docs/source/fedbiomed_node_part_1.md +++ b/docs/source/fedbiomed_node_part_1.md @@ -4,6 +4,12 @@ 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. +## Record start time + +```bash +brainflex record fedbiomed_node_part_1 start +``` + ## Deploy the node Run this to download the images (if needed) and start them: @@ -76,4 +82,13 @@ 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. -Then wait until you hear back before doing [Part 2](fedbiomed_node_part_2). + +## Record end time + +```bash +brainflex record fedbiomed_node_part_1 end +``` + +## Wait + +Once all the above steps are done, wait until you hear back from the FL experiment coordinator before doing [Part 2](fedbiomed_node_part_2). diff --git a/docs/source/fedbiomed_node_part_2.md b/docs/source/fedbiomed_node_part_2.md index 70ced2d..edd6372 100644 --- a/docs/source/fedbiomed_node_part_2.md +++ b/docs/source/fedbiomed_node_part_2.md @@ -1,5 +1,11 @@ # Fed-BioMed node: Part 2 +## Record start time + +```bash +brainflex record fedbiomed_node_part_2 start +``` + ## Check VPN connection Check that the VPN server and the researcher are reachable: @@ -35,3 +41,9 @@ docker compose exec -u fedbiomed node fedbiomed node -p /fbm-node training-plan ``` Once both training plans are approved, the node is ready for training! + +## Record end time + +```bash +brainflex record fedbiomed_node_part_2 end +``` diff --git a/docs/source/fl_ready_dataset.md b/docs/source/fl_ready_dataset.md index d5478fc..d90ea33 100644 --- a/docs/source/fl_ready_dataset.md +++ b/docs/source/fl_ready_dataset.md @@ -1,5 +1,11 @@ # FL-ready dataset +## Record start time + +```bash +brainflex record fl_ready_data start +``` + ## Pipeline installation Install the Nipoppy-to-Fed-BioMed conversion pipeline: @@ -51,3 +57,9 @@ nipoppy_study/derivatives/nipoppy2fedbiomed ├── dataset.csv └── dataset_info.json ``` + +## Record end time and answer questions + +```bash +brainflex record fl_ready_data end +``` diff --git a/docs/source/freesurfer_idps.md b/docs/source/freesurfer_idps.md index 73ca113..dc4ac7f 100644 --- a/docs/source/freesurfer_idps.md +++ b/docs/source/freesurfer_idps.md @@ -1,5 +1,11 @@ # FreeSurfer IDPs +## Record start time + +```bash +brainflex record idp_extraction start +``` + ## Install pipelines Install the FreeSurfer and FreeSurfer stats extraction pipelines: @@ -95,3 +101,9 @@ nipoppy_study/derivatives/freesurfer/7.3.2/idp ├── fs7.3.2-aparc-thickness.tsv └── fs7.3.2-aseg-volume.tsv ``` + +## Record end time and answer questions + +```bash +brainflex record idp_extraction end +``` diff --git a/docs/source/phenotypic_files.md b/docs/source/phenotypic_files.md index 2a14d4d..cdb6cad 100644 --- a/docs/source/phenotypic_files.md +++ b/docs/source/phenotypic_files.md @@ -1,5 +1,11 @@ # Phenotypic files +## Record start time + +```bash +brainflex record semantic_harmonization start +``` + ## Annotation Download the following files to your local machine: @@ -49,3 +55,9 @@ sub-8 ses-unnamed snomed:248152002 55.0 sub-9 ses-unnamed snomed:248153007 60.0 ``` + +## Record end time and answer questions + +```bash +brainflex record semantic_harmonization end +``` From 2a9d2e3409012f7bb7fa42d114b36976d2b9ed39 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 17:43:38 -0500 Subject: [PATCH 06/16] fix failing test --- tests/cli/test_record.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index 4cb86a2..c386ec8 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -19,6 +19,11 @@ ) +def _unwrap(text: str) -> str: + """Collapse whitespace so that prompts wrapped by rich can be matched.""" + return " ".join(text.split()) + + @pytest.fixture def mock_record(mocker: MockerFixture) -> MagicMock: return mocker.patch("brainflex.cli.record.record") @@ -53,9 +58,10 @@ def test_end_other( input=user_input, ) assert result.exit_code == 0 - assert QUESTION_NOTES in result.stdout - assert QUESTION_TOOLS_EASIER not in result.stdout - assert QUESTION_TIME_WITHOUT_TOOLS not in result.stdout + stdout = _unwrap(result.stdout) + assert QUESTION_NOTES in stdout + assert QUESTION_TOOLS_EASIER not in stdout + assert QUESTION_TIME_WITHOUT_TOOLS not in stdout mock_record.assert_called_once_with( dpath_root.resolve(), Step.FEDBIOMED_NODE_PART_1, @@ -75,7 +81,7 @@ def test_end_data_prep( input="4\n1-4h\nwent fine\n", ) assert result.exit_code == 0 - stdout = result.stdout + stdout = _unwrap(result.stdout) # questions are asked in order, with the free-form question last assert ( stdout.index(QUESTION_TOOLS_EASIER) From 338059e66b0538f5b6c4958e7dfb38c4e36c01ce Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Tue, 3 Nov 2026 17:56:39 -0500 Subject: [PATCH 07/16] change printed output line --- src/brainflex/cli/record.py | 6 +++-- src/brainflex/core/record.py | 29 ++++++++++++-------- tests/cli/test_record.py | 52 ++++++++++++++++++++++++++++++------ tests/core/test_record.py | 15 +++++++---- 4 files changed, 76 insertions(+), 26 deletions(-) diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index 905f2c0..b0cb0ce 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -33,5 +33,7 @@ def record_cli( answers[question] = Prompt.ask( f"{question}{hint}", choices=list(choices) ) - record(path, step, status, answers=answers) - typer.echo(f"Recorded {step} {status}") + fpath_timestamps, fpath_survey = record(path, step, status, answers=answers) + typer.echo(f"Saved 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 bc5e78e..775633e 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -88,8 +88,11 @@ def record( step: Step, status: Status, answers: Mapping[str, str] | None = None, -) -> None: - """Record the start or end of a step + some survey answers.""" +) -> tuple[Path, Path | None]: + """Record the start or end of a step + some survey answers. + + Returns the paths to the timestamps file and to the survey file (if applicable). + """ answers = answers or {} questions = _check_answers(step, status, answers) @@ -111,12 +114,16 @@ def record( } append_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS, [row], df=df_timestamps) - if questions: - append_tsv( - dpath / STEP_SURVEY_FILENAME, - STEP_SURVEY_COLUMNS, - ( - {COL_STEP: str(step), COL_QUESTION: q, COL_ANSWER: answers[q]} - for q in questions - ), - ) + if not questions: + return fpath_timestamps, None + + fpath_survey = dpath / STEP_SURVEY_FILENAME + append_tsv( + fpath_survey, + STEP_SURVEY_COLUMNS, + ( + {COL_STEP: str(step), COL_QUESTION: q, COL_ANSWER: answers[q]} + for q in questions + ), + ) + return fpath_timestamps, fpath_survey diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index c386ec8..888a7bc 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -24,9 +24,17 @@ def _unwrap(text: str) -> str: return " ".join(text.split()) +FPATH_TIMESTAMPS = Path("timestamps.tsv") +FPATH_SURVEY = Path("step_survey.tsv") + + +def _fake_record(*args, answers: dict[str, str], **kwargs) -> tuple[Path, Path | None]: + return FPATH_TIMESTAMPS, FPATH_SURVEY if answers else None + + @pytest.fixture def mock_record(mocker: MockerFixture) -> MagicMock: - return mocker.patch("brainflex.cli.record.record") + return mocker.patch("brainflex.cli.record.record", side_effect=_fake_record) class TestRecord: @@ -36,7 +44,11 @@ def test_start(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path ) assert result.exit_code == 0 assert "?" not in result.stdout - assert "Recorded idp_extraction start" in result.stdout + assert ( + f"Saved timestamp for (idp_extraction, start) to {FPATH_TIMESTAMPS}" + in result.stdout + ) + assert "Saved answers" not in result.stdout mock_record.assert_called_once_with( dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} ) @@ -62,6 +74,11 @@ def test_end_other( assert QUESTION_NOTES in stdout assert QUESTION_TOOLS_EASIER not in stdout assert QUESTION_TIME_WITHOUT_TOOLS not in stdout + assert ( + f"Saved timestamp for (fedbiomed_node_part_1, end) to {FPATH_TIMESTAMPS}" + in result.stdout + ) + assert f"Saved answers to {FPATH_SURVEY}" in result.stdout mock_record.assert_called_once_with( dpath_root.resolve(), Step.FEDBIOMED_NODE_PART_1, @@ -88,6 +105,10 @@ def test_end_data_prep( < stdout.index(QUESTION_TIME_WITHOUT_TOOLS) < stdout.index(QUESTION_NOTES) ) + assert ( + f"Saved timestamp for ({step}, end) to {FPATH_TIMESTAMPS}" in result.stdout + ) + assert f"Saved answers to {FPATH_SURVEY}" in result.stdout mock_record.assert_called_once_with( dpath_root.resolve(), step, @@ -170,9 +191,11 @@ def test_invalid_choice( mock_record.assert_not_called() def test_warning(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path): - mock_record.side_effect = lambda *args, **kwargs: warnings.warn( - "step idp_extraction was already started", stacklevel=2 - ) + def _warn(*args, **kwargs) -> tuple[Path, Path | None]: + warnings.warn("step idp_extraction was already started", stacklevel=2) + return _fake_record(*args, **kwargs) + + mock_record.side_effect = _warn result = runner.invoke( app, ["record", "idp_extraction", "start", "--project", str(dpath_root)] ) @@ -183,9 +206,22 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): args = ["record", "fl_ready_data", "--project", str(dpath_root)] args_start = [*args[:2], "start", *args[2:]] args_end = [*args[:2], "end", *args[2:]] - assert runner.invoke(app, args_start).exit_code == 0 - assert runner.invoke(app, args_end, input="3\n<1h\nok\n").exit_code == 0 - dpath = dpath_root / BRAINFLEX_DIRNAME + dpath = dpath_root.resolve() / BRAINFLEX_DIRNAME + result = runner.invoke(app, args_start) + assert result.exit_code == 0 + fpath_timestamps = dpath / TIMESTAMPS_FILENAME + assert ( + f"Saved timestamp for (fl_ready_data, start) to {fpath_timestamps}" + in result.stdout + ) + assert "Saved answers" not in result.stdout + result = runner.invoke(app, args_end, input="3\n<1h\nok\n") + assert result.exit_code == 0 + assert ( + f"Saved timestamp for (fl_ready_data, end) to {fpath_timestamps}" + in result.stdout + ) + assert f"Saved answers to {dpath / STEP_SURVEY_FILENAME}" in result.stdout df_timestamps = pd.read_csv(dpath / TIMESTAMPS_FILENAME, sep="\t", dtype=str) assert list(df_timestamps.columns) == ["timestamp", "step", "status"] assert list(df_timestamps["status"]) == ["start", "end"] diff --git a/tests/core/test_record.py b/tests/core/test_record.py index b54fcd2..c6998e5 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -66,7 +66,10 @@ def test_other(self, step: Step): class TestRecord: def test_start(self, root: Path, fpath_timestamps: Path, fpath_survey: Path): - record(root, Step.IDP_EXTRACTION, Status.START) + assert record(root, Step.IDP_EXTRACTION, Status.START) == ( + fpath_timestamps, + None, + ) df = _read(fpath_timestamps) assert tuple(df.columns) == TIMESTAMPS_COLUMNS assert len(df) == 1 @@ -80,7 +83,9 @@ def test_end_data_prep( self, root: Path, fpath_timestamps: Path, fpath_survey: Path ): record(root, Step.IDP_EXTRACTION, Status.START) - record(root, Step.IDP_EXTRACTION, Status.END, answers=DATA_PREP_ANSWERS) + assert record( + root, Step.IDP_EXTRACTION, Status.END, answers=DATA_PREP_ANSWERS + ) == (fpath_timestamps, fpath_survey) assert list(_read(fpath_timestamps)["status"]) == ["start", "end"] assert fpath_timestamps.read_text().count("timestamp") == 1 df = _read(fpath_survey) @@ -90,11 +95,11 @@ def test_end_data_prep( for q, a in DATA_PREP_ANSWERS.items() ] - def test_end_other(self, root: Path, fpath_survey: Path): + def test_end_other(self, root: Path, fpath_timestamps: Path, fpath_survey: Path): record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) - record( + assert record( root, Step.FEDBIOMED_NODE_PART_1, Status.END, answers={QUESTION_NOTES: ""} - ) + ) == (fpath_timestamps, fpath_survey) assert _read(fpath_survey).to_dict("records") == [ {"step": "fedbiomed_node_part_1", "question": QUESTION_NOTES, "answer": ""} ] From b76f32cfb9beb2c850e142ad3c8d0a63e44d358d Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 09:57:56 -0500 Subject: [PATCH 08/16] add label to every choice + include labels in TSV file --- src/brainflex/cli/console.py | 15 ++++++++++++++- src/brainflex/cli/initial_survey.py | 11 ++++++++--- src/brainflex/cli/record.py | 19 +++++++++++-------- src/brainflex/core/initial_survey.py | 7 ++++++- src/brainflex/core/record.py | 18 +++++++++++++++++- src/brainflex/utils/constants.py | 14 ++++++++++++++ src/brainflex/utils/likert.py | 23 +++++++++++++++++++++++ tests/cli/test_console.py | 18 +++++++++++++++++- tests/cli/test_initial_survey.py | 11 ++++++++--- tests/cli/test_record.py | 7 ++++++- tests/core/test_initial_survey.py | 14 +++++++++++--- tests/core/test_record.py | 17 +++++++++++++++-- tests/utils/test_likert.py | 23 +++++++++++++++++++++++ 13 files changed, 173 insertions(+), 24 deletions(-) create mode 100644 src/brainflex/utils/likert.py create mode 100644 tests/utils/test_likert.py diff --git a/src/brainflex/cli/console.py b/src/brainflex/cli/console.py index da59f02..4e93aa1 100644 --- a/src/brainflex/cli/console.py +++ b/src/brainflex/cli/console.py @@ -1,12 +1,14 @@ """Console output shared across CLI commands.""" import warnings -from collections.abc import Iterator +from collections.abc import Iterator, Sequence from contextlib import contextmanager from rich.console import Console from rich.markup import escape +from brainflex.utils.constants import LIKERT_CHOICES + console = Console(stderr=True) @@ -15,6 +17,17 @@ def print_error(message: object) -> None: console.print(f"[red]ERROR:[/red] {escape(str(message))}", soft_wrap=True) +def format_scale(labels: Sequence[str]) -> str: + """Format a Likert scale legend with one line per choice. + + Raises ValueError if there is not exactly one label per choice. + """ + return "\n".join( + f" {choice} = {label}" + for choice, label in zip(LIKERT_CHOICES, labels, strict=True) + ) + + def _show_warning(message: Warning | str, *args: object, **kwargs: object) -> None: """Print a warning message to stderr.""" console.print(f"[yellow]WARNING:[/yellow] {escape(str(message))}", soft_wrap=True) diff --git a/src/brainflex/cli/initial_survey.py b/src/brainflex/cli/initial_survey.py index 7cbfc07..dd6aa44 100644 --- a/src/brainflex/cli/initial_survey.py +++ b/src/brainflex/cli/initial_survey.py @@ -6,10 +6,14 @@ import typer from rich.prompt import Prompt -from brainflex.cli.console import print_error +from brainflex.cli.console import format_scale, print_error from brainflex.cli.options import RootOption from brainflex.core.initial_survey import check_initial_survey_path, initial_survey -from brainflex.utils.constants import INITIAL_SURVEY_QUESTIONS, LIKERT_CHOICES +from brainflex.utils.constants import ( + FAMILIARITY_LABELS, + INITIAL_SURVEY_QUESTIONS, + LIKERT_CHOICES, +) def initial_survey_cli( @@ -27,7 +31,8 @@ def initial_survey_cli( print_error(e) raise typer.Exit(1) from e - typer.echo("Rate your familiarity (1 = not at all familiar, 5 = very familiar)") + typer.echo("Rate your familiarity with each tool:") + typer.echo(format_scale(FAMILIARITY_LABELS)) answers = { question: int(Prompt.ask(question, choices=LIKERT_CHOICES)) for question in INITIAL_SURVEY_QUESTIONS diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index b0cb0ce..020eb21 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -6,11 +6,15 @@ import typer from rich.prompt import Prompt +from brainflex.cli.console import format_scale from brainflex.cli.options import RootOption -from brainflex.core.record import Status, Step, get_step_questions, record -from brainflex.utils.constants import QUESTION_TOOLS_EASIER - -HINTS = {QUESTION_TOOLS_EASIER: "1 = not at all, 5 = a lot"} +from brainflex.core.record import ( + SCALE_LABELS, + Status, + Step, + get_step_questions, + record, +) def record_cli( @@ -29,10 +33,9 @@ def record_cli( show_default=False, ) else: - hint = f" ({HINTS[question]})" if question in HINTS else "" - answers[question] = Prompt.ask( - f"{question}{hint}", choices=list(choices) - ) + if question in SCALE_LABELS: + typer.echo(format_scale(SCALE_LABELS[question])) + answers[question] = Prompt.ask(question, choices=list(choices)) fpath_timestamps, fpath_survey = record(path, step, status, answers=answers) typer.echo(f"Saved timestamp for ({step}, {status}) to {fpath_timestamps}") if fpath_survey is not None: diff --git a/src/brainflex/core/initial_survey.py b/src/brainflex/core/initial_survey.py index aec71f3..8b19dbb 100644 --- a/src/brainflex/core/initial_survey.py +++ b/src/brainflex/core/initial_survey.py @@ -9,11 +9,13 @@ BRAINFLEX_DIRNAME, COL_ANSWER, COL_QUESTION, + FAMILIARITY_LABELS, INITIAL_SURVEY_FILENAME, INITIAL_SURVEY_QUESTIONS, LIKERT_CHOICES, SURVEY_COLUMNS, ) +from brainflex.utils.likert import label_likert_answer def check_initial_survey_path(root: Path, overwrite: bool = False) -> Path: @@ -55,7 +57,10 @@ def initial_survey( fpath = check_initial_survey_path(root, overwrite=overwrite) rows = [ - {COL_QUESTION: question, COL_ANSWER: answers[question]} + { + COL_QUESTION: question, + COL_ANSWER: label_likert_answer(answers[question], FAMILIARITY_LABELS), + } for question in INITIAL_SURVEY_QUESTIONS ] pd.DataFrame(rows, columns=SURVEY_COLUMNS).to_csv(fpath, sep="\t", index=False) diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 775633e..7a4f3a5 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -22,7 +22,9 @@ TIME_WITHOUT_TOOLS_CHOICES, TIMESTAMPS_COLUMNS, TIMESTAMPS_FILENAME, + TOOLS_EASIER_LABELS, ) +from brainflex.utils.likert import label_likert_answer from brainflex.utils.tsv import append_tsv, load_tsv @@ -55,6 +57,9 @@ class Status(StrEnum): } COMMON_QUESTIONS: dict[str, tuple[str, ...] | None] = {QUESTION_NOTES: None} +# labels of Likert scale questions +SCALE_LABELS: dict[str, tuple[str, ...]] = {QUESTION_TOOLS_EASIER: TOOLS_EASIER_LABELS} + 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.""" @@ -83,6 +88,13 @@ def _check_answers( return questions +def _format_answer(question: str, answer: str) -> str: + """Add the label to the answer of a Likert scale question.""" + if question in SCALE_LABELS: + return label_likert_answer(answer, SCALE_LABELS[question]) + return answer + + def record( root: Path, step: Step, @@ -122,7 +134,11 @@ def record( fpath_survey, STEP_SURVEY_COLUMNS, ( - {COL_STEP: str(step), COL_QUESTION: q, COL_ANSWER: answers[q]} + { + COL_STEP: str(step), + COL_QUESTION: q, + COL_ANSWER: _format_answer(q, answers[q]), + } for q in questions ), ) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index 276c5ea..92b62a1 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -9,6 +9,20 @@ COL_ANSWER = "answer" SURVEY_COLUMNS = (COL_QUESTION, COL_ANSWER) LIKERT_CHOICES = ("1", "2", "3", "4", "5") +FAMILIARITY_LABELS = ( + "Never heard of it before", + "Have heard of it but never used", + "Have used but only with example/toy data", + "Have used with real data but not regularly", + "Regularly use/used with non-toy data", +) +TOOLS_EASIER_LABELS = ( + "Not at all", + "Slightly", + "Moderately", + "Considerably", + "Greatly", +) BACKUP_TIMESTAMP_FORMAT = "%Y%m%dT%H%M%S" # initial-survey diff --git a/src/brainflex/utils/likert.py b/src/brainflex/utils/likert.py new file mode 100644 index 0000000..03076a5 --- /dev/null +++ b/src/brainflex/utils/likert.py @@ -0,0 +1,23 @@ +"""Likert scale utilities.""" + +from collections.abc import Sequence + +from brainflex.utils.constants import LIKERT_CHOICES + + +def label_likert_answer(answer: str | int, labels: Sequence[str]) -> str: + """Return a Likert scale answer with its label. + + Raises ValueError if the answer is not a valid choice or if there is not + exactly one label per choice. + """ + if len(labels) != len(LIKERT_CHOICES): + raise ValueError( + f"Expected {len(LIKERT_CHOICES)} labels, got {len(labels)}: {labels}" + ) + answer = str(answer) + if answer not in LIKERT_CHOICES: + raise ValueError( + f"Invalid answer {answer!r}: must be one of {', '.join(LIKERT_CHOICES)}" + ) + return f"{answer} ({labels[LIKERT_CHOICES.index(answer)]})" diff --git a/tests/cli/test_console.py b/tests/cli/test_console.py index dd89c33..9b9d541 100644 --- a/tests/cli/test_console.py +++ b/tests/cli/test_console.py @@ -2,7 +2,23 @@ import pytest -from brainflex.cli.console import print_error, render_warnings +from brainflex.cli.console import format_scale, print_error, render_warnings + + +class TestFormatScale: + def test_format_scale(self): + assert format_scale(["a", "b", "c", "d", "e"]).splitlines() == [ + " 1 = a", + " 2 = b", + " 3 = c", + " 4 = d", + " 5 = e", + ] + + @pytest.mark.parametrize("labels", [["a", "b", "c", "d"], list("abcdef")]) + def test_wrong_number_of_labels(self, labels: list[str]): + with pytest.raises(ValueError, match="zip"): + format_scale(labels) class TestPrintError: diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py index ac7acbb..865a458 100644 --- a/tests/cli/test_initial_survey.py +++ b/tests/cli/test_initial_survey.py @@ -8,6 +8,7 @@ from brainflex.cli.main import app from brainflex.utils.constants import ( BRAINFLEX_DIRNAME, + FAMILIARITY_LABELS, INITIAL_SURVEY_FILENAME, INITIAL_SURVEY_QUESTIONS, ) @@ -39,6 +40,10 @@ def test_initial_survey( assert result.exit_code == 0 for tool in ("Nipoppy", "Neurobagel", "Fed-BioMed"): assert f"How familiar are you with {tool}?" in result.stdout + # legend is printed once, before the questions + for i, label in enumerate(FAMILIARITY_LABELS, start=1): + assert result.stdout.count(f"{i} = {label}") == 1 + assert result.stdout.index(label) < result.stdout.index("How familiar") assert "Saved answers to" in result.stdout mock_initial_survey.assert_called_once_with( dpath_root.resolve(), _answers(1, 3, 5), overwrite=False @@ -114,7 +119,7 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Pat assert result.exit_code == 0 assert fpath_survey.read_text().splitlines() == [ "question\tanswer", - "How familiar are you with Nipoppy?\t1", - "How familiar are you with Neurobagel?\t3", - "How familiar are you with Fed-BioMed?\t5", + "How familiar are you with Nipoppy?\t1 (Never heard of it before)", + "How familiar are you with Neurobagel?\t3 (Have used but only with example/toy data)", + "How familiar are you with Fed-BioMed?\t5 (Regularly use/used with non-toy data)", ] diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index 888a7bc..7af51a6 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -16,6 +16,7 @@ QUESTION_TOOLS_EASIER, STEP_SURVEY_FILENAME, TIMESTAMPS_FILENAME, + TOOLS_EASIER_LABELS, ) @@ -74,6 +75,7 @@ def test_end_other( assert QUESTION_NOTES in stdout assert QUESTION_TOOLS_EASIER not in stdout assert QUESTION_TIME_WITHOUT_TOOLS not in stdout + assert not any(label in stdout for label in TOOLS_EASIER_LABELS) assert ( f"Saved timestamp for (fedbiomed_node_part_1, end) to {FPATH_TIMESTAMPS}" in result.stdout @@ -105,6 +107,9 @@ def test_end_data_prep( < stdout.index(QUESTION_TIME_WITHOUT_TOOLS) < stdout.index(QUESTION_NOTES) ) + # each scale point is labelled before the question + for i, label in enumerate(TOOLS_EASIER_LABELS, start=1): + assert stdout.index(f"{i} = {label}") < stdout.index(QUESTION_TOOLS_EASIER) assert ( f"Saved timestamp for ({step}, end) to {FPATH_TIMESTAMPS}" in result.stdout ) @@ -227,7 +232,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}\t3", + f"fl_ready_data\t{QUESTION_TOOLS_EASIER}\t3 (Moderately)", f"fl_ready_data\t{QUESTION_TIME_WITHOUT_TOOLS}\t<1h", f"fl_ready_data\t{QUESTION_NOTES}\tok", ] diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py index eaa98c9..26e90b5 100644 --- a/tests/core/test_initial_survey.py +++ b/tests/core/test_initial_survey.py @@ -55,14 +55,22 @@ def test_creates_file(self, root: Path, fpath_survey: Path): "How familiar are you with Neurobagel?", "How familiar are you with Fed-BioMed?", ] - assert list(df["answer"]) == ["1", "3", "5"] + assert list(df["answer"]) == [ + "1 (Never heard of it before)", + "3 (Have used but only with example/toy data)", + "5 (Regularly use/used with non-toy data)", + ] def test_question_order(self, root: Path, fpath_survey: Path): answers = dict(reversed(_answers(1, 3, 5).items())) initial_survey(root, answers) df = _read(fpath_survey) assert list(df["question"]) == list(INITIAL_SURVEY_QUESTIONS) - assert list(df["answer"]) == ["1", "3", "5"] + assert list(df["answer"]) == [ + "1 (Never heard of it before)", + "3 (Have used but only with example/toy data)", + "5 (Regularly use/used with non-toy data)", + ] def test_exists(self, root: Path, fpath_survey: Path): fpath_survey.write_text("existing") @@ -75,7 +83,7 @@ def test_overwrite(self, root: Path, fpath_survey: Path): initial_survey(root, _answers(2, 2, 2), overwrite=True) df = _read(fpath_survey) assert len(df) == len(INITIAL_SURVEY_QUESTIONS) - assert list(df["answer"]) == ["2", "2", "2"] + assert list(df["answer"]) == ["2 (Have heard of it but never used)"] * 3 @pytest.mark.parametrize("ratings", [(0, 1, 1), (1, 6, 1), (1, 1, -1)]) def test_invalid_rating( diff --git a/tests/core/test_record.py b/tests/core/test_record.py index c6998e5..332027f 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -91,8 +91,21 @@ def test_end_data_prep( df = _read(fpath_survey) assert tuple(df.columns) == STEP_SURVEY_COLUMNS assert df.to_dict("records") == [ - {"step": "idp_extraction", "question": q, "answer": a} - for q, a in DATA_PREP_ANSWERS.items() + { + "step": "idp_extraction", + "question": QUESTION_TOOLS_EASIER, + "answer": "4 (Considerably)", + }, + { + "step": "idp_extraction", + "question": QUESTION_TIME_WITHOUT_TOOLS, + "answer": "1-4h", + }, + { + "step": "idp_extraction", + "question": QUESTION_NOTES, + "answer": "went fine", + }, ] def test_end_other(self, root: Path, fpath_timestamps: Path, fpath_survey: Path): diff --git a/tests/utils/test_likert.py b/tests/utils/test_likert.py new file mode 100644 index 0000000..eb005db --- /dev/null +++ b/tests/utils/test_likert.py @@ -0,0 +1,23 @@ +import pytest + +from brainflex.utils.likert import label_likert_answer + +LABELS = ("a", "b", "c", "d", "e") + + +class TestLabelLikertAnswer: + @pytest.mark.parametrize( + ("answer", "expected"), [("1", "1 (a)"), (3, "3 (c)"), ("5", "5 (e)")] + ) + def test_label(self, answer: str | int, expected: str): + assert label_likert_answer(answer, LABELS) == expected + + @pytest.mark.parametrize("answer", ["0", "6", 6, "", "a"]) + def test_invalid_answer(self, answer: str | int): + with pytest.raises(ValueError, match="must be one of"): + label_likert_answer(answer, LABELS) + + @pytest.mark.parametrize("labels", [LABELS[:4], (*LABELS, "f")]) + def test_wrong_number_of_labels(self, labels: tuple[str, ...]): + with pytest.raises(ValueError, match="Expected 5 labels"): + label_likert_answer("1", labels) From e9527e496c0b37ce81ce2aa655a36ac13ff945d2 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 11:24:57 -0500 Subject: [PATCH 09/16] change familiarity labels --- src/brainflex/utils/constants.py | 10 +++++----- tests/cli/test_initial_survey.py | 4 ++-- tests/core/test_initial_survey.py | 10 +++++----- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index 92b62a1..2c04a1c 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -10,11 +10,11 @@ SURVEY_COLUMNS = (COL_QUESTION, COL_ANSWER) LIKERT_CHOICES = ("1", "2", "3", "4", "5") FAMILIARITY_LABELS = ( - "Never heard of it before", - "Have heard of it but never used", - "Have used but only with example/toy data", - "Have used with real data but not regularly", - "Regularly use/used with non-toy data", + "I have never heard of it before this project", + "I have heard of it but never used", + "I have used but only with example/toy data", + "I have used with real data but not regularly", + "I use it regularly / It is one of my go-to tools", ) TOOLS_EASIER_LABELS = ( "Not at all", diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py index 865a458..ed4469e 100644 --- a/tests/cli/test_initial_survey.py +++ b/tests/cli/test_initial_survey.py @@ -120,6 +120,6 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Pat assert fpath_survey.read_text().splitlines() == [ "question\tanswer", "How familiar are you with Nipoppy?\t1 (Never heard of it before)", - "How familiar are you with Neurobagel?\t3 (Have used but only with example/toy data)", - "How familiar are you with Fed-BioMed?\t5 (Regularly use/used with non-toy data)", + "How familiar are you with Neurobagel?\t3 (I have used but only with example/toy data)", + "How familiar are you with Fed-BioMed?\t5 (I use it regularly / It is one of my go-to tools)", ] diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py index 26e90b5..254c75c 100644 --- a/tests/core/test_initial_survey.py +++ b/tests/core/test_initial_survey.py @@ -57,8 +57,8 @@ def test_creates_file(self, root: Path, fpath_survey: Path): ] assert list(df["answer"]) == [ "1 (Never heard of it before)", - "3 (Have used but only with example/toy data)", - "5 (Regularly use/used with non-toy data)", + "3 (I have used but only with example/toy data)", + "5 (I use it regularly / It is one of my go-to tools)", ] def test_question_order(self, root: Path, fpath_survey: Path): @@ -68,8 +68,8 @@ def test_question_order(self, root: Path, fpath_survey: Path): assert list(df["question"]) == list(INITIAL_SURVEY_QUESTIONS) assert list(df["answer"]) == [ "1 (Never heard of it before)", - "3 (Have used but only with example/toy data)", - "5 (Regularly use/used with non-toy data)", + "3 (I have used but only with example/toy data)", + "5 (I use it regularly / It is one of my go-to tools)", ] def test_exists(self, root: Path, fpath_survey: Path): @@ -83,7 +83,7 @@ def test_overwrite(self, root: Path, fpath_survey: Path): initial_survey(root, _answers(2, 2, 2), overwrite=True) df = _read(fpath_survey) assert len(df) == len(INITIAL_SURVEY_QUESTIONS) - assert list(df["answer"]) == ["2 (Have heard of it but never used)"] * 3 + assert list(df["answer"]) == ["2 (I have heard of it but never used)"] * 3 @pytest.mark.parametrize("ratings", [(0, 1, 1), (1, 6, 1), (1, 1, -1)]) def test_invalid_rating( From 197d581a330a50e72aa809c74cf79123c9ef16d9 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:07:05 -0500 Subject: [PATCH 10/16] move warning, binarize first step question, and print blurb for data prep steps --- src/brainflex/cli/record.py | 11 +++-- src/brainflex/core/record.py | 70 ++++++++++++++++++--------- src/brainflex/utils/constants.py | 10 +--- tests/cli/test_record.py | 48 +++++++++++-------- tests/core/test_record.py | 81 +++++++++++++++++++++++--------- 5 files changed, 143 insertions(+), 77 deletions(-) diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index 020eb21..41a2ffc 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -6,13 +6,13 @@ import typer from rich.prompt import Prompt -from brainflex.cli.console import format_scale from brainflex.cli.options import RootOption from brainflex.core.record import ( - SCALE_LABELS, Status, Step, + check_status, get_step_questions, + get_step_summary, record, ) @@ -23,8 +23,13 @@ def record_cli( path: RootOption = Path(), ) -> None: """Record the start or end of an experiment step.""" + # warn before asking questions + check_status(path, step, status) + answers = {} if status == Status.END: + if (summary := get_step_summary(step)) is not None: + typer.echo(f"{summary}\n") for question, choices in get_step_questions(step).items(): if choices is None: answers[question] = Prompt.ask( @@ -33,8 +38,6 @@ def record_cli( show_default=False, ) else: - if question in SCALE_LABELS: - typer.echo(format_scale(SCALE_LABELS[question])) answers[question] = Prompt.ask(question, choices=list(choices)) fpath_timestamps, fpath_survey = record(path, step, status, answers=answers) typer.echo(f"Saved timestamp for ({step}, {status}) to {fpath_timestamps}") diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 7a4f3a5..a93b7d5 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -13,7 +13,6 @@ COL_STATUS, COL_STEP, COL_TIMESTAMP, - LIKERT_CHOICES, QUESTION_NOTES, QUESTION_TIME_WITHOUT_TOOLS, QUESTION_TOOLS_EASIER, @@ -22,9 +21,8 @@ TIME_WITHOUT_TOOLS_CHOICES, TIMESTAMPS_COLUMNS, TIMESTAMPS_FILENAME, - TOOLS_EASIER_LABELS, + TOOLS_EASIER_CHOICES, ) -from brainflex.utils.likert import label_likert_answer from brainflex.utils.tsv import append_tsv, load_tsv @@ -52,13 +50,35 @@ class Status(StrEnum): # 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: LIKERT_CHOICES, + QUESTION_TOOLS_EASIER: TOOLS_EASIER_CHOICES, QUESTION_TIME_WITHOUT_TOOLS: TIME_WITHOUT_TOOLS_CHOICES, } COMMON_QUESTIONS: dict[str, tuple[str, ...] | None] = {QUESTION_NOTES: None} -# labels of Likert scale questions -SCALE_LABELS: dict[str, tuple[str, ...]] = {QUESTION_TOOLS_EASIER: TOOLS_EASIER_LABELS} +# 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 extract " + "imaging-derived phenotypes (IDPs) (cortical thickness, subcortical volumes, " + "etc.) from existing FreeSurfer subject directories and 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 " + "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_DATA: ( + "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 " + "participating in the FL experiment will have datasets with the same structure " + "and column names, so that they can be used together to train models." + ), +} def get_step_questions(step: Step) -> dict[str, tuple[str, ...] | None]: @@ -68,6 +88,11 @@ def get_step_questions(step: Step) -> dict[str, tuple[str, ...] | None]: return dict(COMMON_QUESTIONS) +def get_step_summary(step: Step) -> str | None: + """Return a high-level summary of a data preparation step (None otherwise).""" + return STEP_SUMMARIES.get(step) + + def _check_answers( step: Step, status: Status, answers: Mapping[str, str] ) -> dict[str, tuple[str, ...] | None]: @@ -88,11 +113,17 @@ def _check_answers( return questions -def _format_answer(question: str, answer: str) -> str: - """Add the label to the answer of a Likert scale question.""" - if question in SCALE_LABELS: - return label_likert_answer(answer, SCALE_LABELS[question]) - return answer +def check_status(root: Path, step: Step, status: Status) -> None: + """Warn if a step is started twice or ended without being started.""" + df_timestamps = load_tsv( + root / BRAINFLEX_DIRNAME / TIMESTAMPS_FILENAME, TIMESTAMPS_COLUMNS + ) + statuses = df_timestamps.query(f"{COL_STEP} == @step")[COL_STATUS] + last_status = statuses.iloc[-1] if not statuses.empty else None + if status == Status.START and last_status == Status.START: + warnings.warn(f"step {step} was already started", stacklevel=2) + elif status == Status.END and last_status != Status.START: + warnings.warn(f"step {step} was not started", stacklevel=2) def record( @@ -103,28 +134,21 @@ def record( ) -> tuple[Path, Path | None]: """Record the start or end of a step + some survey answers. - Returns the paths to the timestamps file and to the survey file (if applicable). + Call ``check_status`` first to warn about steps started twice or ended without + being started. Returns the paths to the timestamps file and to the survey file + (if applicable). """ answers = answers or {} questions = _check_answers(step, status, answers) dpath = root / BRAINFLEX_DIRNAME fpath_timestamps = dpath / TIMESTAMPS_FILENAME - df_timestamps = load_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS) - - statuses = df_timestamps.query(f"{COL_STEP} == @step")[COL_STATUS] - last_status = statuses.iloc[-1] if not statuses.empty else None - if status == Status.START and last_status == Status.START: - warnings.warn(f"step {step} was already started", stacklevel=2) - elif status == Status.END and last_status != Status.START: - warnings.warn(f"step {step} was not started", stacklevel=2) - row = { COL_TIMESTAMP: datetime.now().astimezone().isoformat(timespec="seconds"), COL_STEP: str(step), COL_STATUS: str(status), } - append_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS, [row], df=df_timestamps) + append_tsv(fpath_timestamps, TIMESTAMPS_COLUMNS, [row]) if not questions: return fpath_timestamps, None @@ -137,7 +161,7 @@ def record( { COL_STEP: str(step), COL_QUESTION: q, - COL_ANSWER: _format_answer(q, answers[q]), + COL_ANSWER: answers[q], } for q in questions ), diff --git a/src/brainflex/utils/constants.py b/src/brainflex/utils/constants.py index 2c04a1c..d15b033 100644 --- a/src/brainflex/utils/constants.py +++ b/src/brainflex/utils/constants.py @@ -16,13 +16,6 @@ "I have used with real data but not regularly", "I use it regularly / It is one of my go-to tools", ) -TOOLS_EASIER_LABELS = ( - "Not at all", - "Slightly", - "Moderately", - "Considerably", - "Greatly", -) BACKUP_TIMESTAMP_FORMAT = "%Y%m%dT%H%M%S" # initial-survey @@ -41,8 +34,9 @@ STEP_SURVEY_FILENAME = "step_survey.tsv" STEP_SURVEY_COLUMNS = (COL_STEP, *SURVEY_COLUMNS) QUESTION_TOOLS_EASIER = ( - "By how much did the Nipoppy/Neurobagel tools make things easier?" + "Did the Nipoppy/Neurobagel tools make things 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?" ) diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index 7af51a6..c741cd7 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -1,4 +1,3 @@ -import warnings from pathlib import Path from unittest.mock import MagicMock @@ -16,7 +15,6 @@ QUESTION_TOOLS_EASIER, STEP_SURVEY_FILENAME, TIMESTAMPS_FILENAME, - TOOLS_EASIER_LABELS, ) @@ -75,7 +73,6 @@ def test_end_other( assert QUESTION_NOTES in stdout assert QUESTION_TOOLS_EASIER not in stdout assert QUESTION_TIME_WITHOUT_TOOLS not in stdout - assert not any(label in stdout for label in TOOLS_EASIER_LABELS) assert ( f"Saved timestamp for (fedbiomed_node_part_1, end) to {FPATH_TIMESTAMPS}" in result.stdout @@ -97,7 +94,7 @@ def test_end_data_prep( result = runner.invoke( app, ["record", str(step), "end", "--project", str(dpath_root)], - input="4\n1-4h\nwent fine\n", + input="yes\n1-4h\nwent fine\n", ) assert result.exit_code == 0 stdout = _unwrap(result.stdout) @@ -107,9 +104,7 @@ def test_end_data_prep( < stdout.index(QUESTION_TIME_WITHOUT_TOOLS) < stdout.index(QUESTION_NOTES) ) - # each scale point is labelled before the question - for i, label in enumerate(TOOLS_EASIER_LABELS, start=1): - assert stdout.index(f"{i} = {label}") < stdout.index(QUESTION_TOOLS_EASIER) + assert "[yes/no/unsure]" in stdout assert ( f"Saved timestamp for ({step}, end) to {FPATH_TIMESTAMPS}" in result.stdout ) @@ -119,7 +114,7 @@ def test_end_data_prep( step, Status.END, answers={ - QUESTION_TOOLS_EASIER: "4", + QUESTION_TOOLS_EASIER: "yes", QUESTION_TIME_WITHOUT_TOOLS: "1-4h", QUESTION_NOTES: "went fine", }, @@ -131,7 +126,7 @@ def test_end_data_prep_invalid_input_reprompts( result = runner.invoke( app, ["record", "idp_extraction", "end", "--project", str(dpath_root)], - input="6\n5\n2 days\n>7d\n\n", + input="maybe\nunsure\n2 days\n>7d\n\n", ) assert result.exit_code == 0 mock_record.assert_called_once_with( @@ -139,7 +134,7 @@ def test_end_data_prep_invalid_input_reprompts( Step.IDP_EXTRACTION, Status.END, answers={ - QUESTION_TOOLS_EASIER: "5", + QUESTION_TOOLS_EASIER: "unsure", QUESTION_TIME_WITHOUT_TOOLS: ">7d", QUESTION_NOTES: "", }, @@ -195,17 +190,31 @@ def test_invalid_choice( assert result.exit_code == 2 mock_record.assert_not_called() - def test_warning(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path): - def _warn(*args, **kwargs) -> tuple[Path, Path | None]: - warnings.warn("step idp_extraction was already started", stacklevel=2) - return _fake_record(*args, **kwargs) + def test_warning_already_started( + self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path + ): + args = ["record", "idp_extraction", "start", "--project", str(dpath_root)] + mock_record.side_effect = None + (dpath_root / BRAINFLEX_DIRNAME / TIMESTAMPS_FILENAME).write_text( + "timestamp\tstep\tstatus\nt0\tidp_extraction\tstart\n" + ) + mock_record.return_value = (FPATH_TIMESTAMPS, None) + result = runner.invoke(app, args) + assert result.exit_code == 0 + assert "WARNING: step idp_extraction was already started" in result.stderr - mock_record.side_effect = _warn + @pytest.mark.parametrize("step", ["idp_extraction", "fedbiomed_node_part_1"]) + def test_warning_not_started_printed_first( + self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path, step: str + ): result = runner.invoke( - app, ["record", "idp_extraction", "start", "--project", str(dpath_root)] + app, + ["record", step, "end", "--project", str(dpath_root)], + input="yes\n1-4h\n\n", ) assert result.exit_code == 0 - assert "WARNING: step idp_extraction was already started" in result.stderr + assert result.output.startswith(f"WARNING: step {step} was not started\n") + 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)] @@ -220,7 +229,8 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path): in result.stdout ) assert "Saved answers" not in result.stdout - result = runner.invoke(app, args_end, input="3\n<1h\nok\n") + result = runner.invoke(app, args_end, input="yes\n<1h\nok\n") + assert "WARNING" not in result.stderr assert result.exit_code == 0 assert ( f"Saved timestamp for (fl_ready_data, end) to {fpath_timestamps}" @@ -232,7 +242,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}\t3 (Moderately)", + 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", ] diff --git a/tests/core/test_record.py b/tests/core/test_record.py index 332027f..f63b7be 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -9,6 +9,7 @@ DATA_PREP_STEPS, Status, Step, + check_status, get_step_questions, record, ) @@ -24,7 +25,7 @@ ) DATA_PREP_ANSWERS = { - QUESTION_TOOLS_EASIER: "4", + QUESTION_TOOLS_EASIER: "yes", QUESTION_TIME_WITHOUT_TOOLS: "1-4h", QUESTION_NOTES: "went fine", } @@ -94,7 +95,7 @@ def test_end_data_prep( { "step": "idp_extraction", "question": QUESTION_TOOLS_EASIER, - "answer": "4 (Considerably)", + "answer": "yes", }, { "step": "idp_extraction", @@ -171,8 +172,8 @@ def test_invalid_questions( @pytest.mark.parametrize( ("question", "answer"), [ - (QUESTION_TOOLS_EASIER, "0"), - (QUESTION_TOOLS_EASIER, "6"), + (QUESTION_TOOLS_EASIER, "maybe"), + (QUESTION_TOOLS_EASIER, "4"), (QUESTION_TIME_WITHOUT_TOOLS, "2 days"), ], ) @@ -189,22 +190,52 @@ def test_invalid_answer( assert not fpath_timestamps.exists() def test_no_warning(self, root: Path): + # warnings are emitted by check_status with warnings.catch_warnings(): warnings.simplefilter("error") - record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) record( root, Step.FEDBIOMED_NODE_PART_1, Status.END, answers={QUESTION_NOTES: ""}, ) + record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) + record(root, Step.FEDBIOMED_NODE_PART_1, Status.START) + + def test_invalid_timestamps_file(self, root: Path, fpath_timestamps: Path): + fpath_timestamps.write_text("timestamp\tstep\nt0\tidp_extraction\n") + with pytest.warns(UserWarning, match="invalid file"): + record(root, Step.IDP_EXTRACTION, Status.START) + assert len(list(fpath_timestamps.parent.glob("timestamps-*.tsv"))) == 1 + assert len(_read(fpath_timestamps)) == 1 + + +class TestCheckStatus: + def _record(self, root: Path, step: Step, status: Status) -> None: + answers = {QUESTION_NOTES: ""} if status == Status.END else None + record(root, step, status, answers=answers) + + @pytest.mark.parametrize( + ("statuses", "status"), + [ + ([], Status.START), + ([Status.START], Status.END), + ([Status.START, Status.END], Status.START), + ], + ) + def test_no_warning(self, root: Path, statuses: list[Status], status: Status): + for previous in statuses: + self._record(root, Step.FEDBIOMED_NODE_PART_1, previous) + with warnings.catch_warnings(): + warnings.simplefilter("error") + check_status(root, Step.FEDBIOMED_NODE_PART_1, status) @pytest.mark.parametrize( - ("statuses", "message"), + ("statuses", "status", "message"), [ - ([Status.END], "was not started"), - ([Status.START, Status.START], "was already started"), - ([Status.START, Status.END, Status.END], "was not started"), + ([], Status.END, "was not started"), + ([Status.START], Status.START, "was already started"), + ([Status.START, Status.END], Status.END, "was not started"), ], ) def test_warning( @@ -212,25 +243,29 @@ def test_warning( root: Path, fpath_timestamps: Path, statuses: list[Status], + status: Status, message: str, ): - step = Step.FEDBIOMED_NODE_PART_1 - - def _record(status: Status) -> None: - answers = {QUESTION_NOTES: ""} if status == Status.END else None - record(root, step, status, answers=answers) - - for status in statuses[:-1]: - _record(status) + for previous in statuses: + self._record(root, Step.FEDBIOMED_NODE_PART_1, previous) + content = fpath_timestamps.read_text() if statuses else None with pytest.warns(UserWarning, match=message): - _record(statuses[-1]) - assert len(_read(fpath_timestamps)) == len(statuses) + check_status(root, Step.FEDBIOMED_NODE_PART_1, status) + # nothing is written + if content is None: + assert not fpath_timestamps.exists() + else: + assert fpath_timestamps.read_text() == content + + 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) def test_invalid_timestamps_file(self, root: Path, fpath_timestamps: Path): fpath_timestamps.write_text("timestamp\tstep\nt0\tidp_extraction\n") - with pytest.warns(UserWarning, match="invalid file") as record_warnings: - record(root, Step.IDP_EXTRACTION, Status.START) - assert len(list(fpath_timestamps.parent.glob("timestamps-*.tsv"))) == 1 - assert len(_read(fpath_timestamps)) == 1 + with pytest.warns(UserWarning) as record_warnings: + check_status(root, Step.IDP_EXTRACTION, Status.START) # previous records are discarded, so no start/end warning + assert str(record_warnings[0].message).startswith("invalid file") assert len(record_warnings) == 1 From 029e00bcd603a7e7ed858ec5183d1c2450241a9f Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:13:26 -0500 Subject: [PATCH 11/16] improve styling --- src/brainflex/cli/console.py | 7 ++++--- src/brainflex/cli/initial_survey.py | 4 +++- tests/cli/test_console.py | 11 ++++++++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/brainflex/cli/console.py b/src/brainflex/cli/console.py index 4e93aa1..24f6074 100644 --- a/src/brainflex/cli/console.py +++ b/src/brainflex/cli/console.py @@ -6,6 +6,7 @@ from rich.console import Console from rich.markup import escape +from rich.text import Text from brainflex.utils.constants import LIKERT_CHOICES @@ -17,13 +18,13 @@ def print_error(message: object) -> None: console.print(f"[red]ERROR:[/red] {escape(str(message))}", soft_wrap=True) -def format_scale(labels: Sequence[str]) -> str: +def format_scale(labels: Sequence[str]) -> Text: """Format a Likert scale legend with one line per choice. Raises ValueError if there is not exactly one label per choice. """ - return "\n".join( - f" {choice} = {label}" + return Text("\n").join( + Text.assemble(" ", (choice, "prompt.choices"), f" = {label}") for choice, label in zip(LIKERT_CHOICES, labels, strict=True) ) diff --git a/src/brainflex/cli/initial_survey.py b/src/brainflex/cli/initial_survey.py index dd6aa44..53f574f 100644 --- a/src/brainflex/cli/initial_survey.py +++ b/src/brainflex/cli/initial_survey.py @@ -4,6 +4,7 @@ from typing import Annotated import typer +from rich import get_console from rich.prompt import Prompt from brainflex.cli.console import format_scale, print_error @@ -32,7 +33,8 @@ def initial_survey_cli( raise typer.Exit(1) from e typer.echo("Rate your familiarity with each tool:") - typer.echo(format_scale(FAMILIARITY_LABELS)) + # same console as Prompt + get_console().print(format_scale(FAMILIARITY_LABELS), soft_wrap=True) answers = { question: int(Prompt.ask(question, choices=LIKERT_CHOICES)) for question in INITIAL_SURVEY_QUESTIONS diff --git a/tests/cli/test_console.py b/tests/cli/test_console.py index 9b9d541..2d9fba5 100644 --- a/tests/cli/test_console.py +++ b/tests/cli/test_console.py @@ -7,7 +7,7 @@ class TestFormatScale: def test_format_scale(self): - assert format_scale(["a", "b", "c", "d", "e"]).splitlines() == [ + assert format_scale(["a", "b", "c", "d", "e"]).plain.splitlines() == [ " 1 = a", " 2 = b", " 3 = c", @@ -15,6 +15,15 @@ def test_format_scale(self): " 5 = e", ] + def test_choices_styled_like_prompt(self): + text = format_scale(["a", "b", "c", "d", "e"]) + styled = [ + text.plain[span.start : span.end] + for span in text.spans + if span.style == "prompt.choices" + ] + assert styled == ["1", "2", "3", "4", "5"] + @pytest.mark.parametrize("labels", [["a", "b", "c", "d"], list("abcdef")]) def test_wrong_number_of_labels(self, labels: list[str]): with pytest.raises(ValueError, match="zip"): From f8700d828365f703a6d36c7e5168a6784c99987c Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:14:20 -0500 Subject: [PATCH 12/16] more styling --- src/brainflex/cli/record.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index 41a2ffc..02317b2 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -4,7 +4,9 @@ from typing import Annotated import typer +from rich import get_console from rich.prompt import Prompt +from rich.text import Text from brainflex.cli.options import RootOption from brainflex.core.record import ( @@ -29,7 +31,8 @@ def record_cli( answers = {} if status == Status.END: if (summary := get_step_summary(step)) is not None: - typer.echo(f"{summary}\n") + # same console and colour as Prompt choices + get_console().print(Text(summary, style="prompt.choices"), end="\n\n") for question, choices in get_step_questions(step).items(): if choices is None: answers[question] = Prompt.ask( From 8033a6b33b2b781ca4c868020c4037561cedc62b Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:20:23 -0500 Subject: [PATCH 13/16] minor edits to blurb --- src/brainflex/core/record.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index a93b7d5..bd34b32 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -58,11 +58,11 @@ class Status(StrEnum): # 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 extract " + "In 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 and combine them into " + "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)." + "other site participating in the FL experiment)" ), Step.SEMANTIC_HARMONIZATION: ( "In this step, you created a file containing participant IDs, session IDs, and " From c297699fb512ef74f85399567ecf2025b9218560 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:26:15 -0500 Subject: [PATCH 14/16] get timestamp at the start of the command, before any question is asked --- src/brainflex/cli/record.py | 7 +++++- src/brainflex/core/record.py | 5 +++- tests/cli/test_record.py | 47 +++++++++++++++++++++++++++++++++--- tests/core/test_record.py | 9 ++++++- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/brainflex/cli/record.py b/src/brainflex/cli/record.py index 02317b2..55862ce 100644 --- a/src/brainflex/cli/record.py +++ b/src/brainflex/cli/record.py @@ -1,5 +1,6 @@ """CLI for ``brainflex record``.""" +from datetime import datetime from pathlib import Path from typing import Annotated @@ -25,6 +26,8 @@ def record_cli( path: RootOption = Path(), ) -> None: """Record the start or end of an experiment step.""" + timestamp = datetime.now().astimezone() + # warn before asking questions check_status(path, step, status) @@ -42,7 +45,9 @@ def record_cli( ) else: answers[question] = Prompt.ask(question, choices=list(choices)) - fpath_timestamps, fpath_survey = record(path, step, status, answers=answers) + fpath_timestamps, fpath_survey = record( + path, step, status, answers=answers, timestamp=timestamp + ) typer.echo(f"Saved 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 bd34b32..0db0b75 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -131,6 +131,7 @@ def record( step: Step, status: Status, answers: Mapping[str, str] | None = None, + timestamp: datetime | None = None, ) -> tuple[Path, Path | None]: """Record the start or end of a step + some survey answers. @@ -144,7 +145,9 @@ def record( dpath = root / BRAINFLEX_DIRNAME fpath_timestamps = dpath / TIMESTAMPS_FILENAME row = { - COL_TIMESTAMP: datetime.now().astimezone().isoformat(timespec="seconds"), + COL_TIMESTAMP: (timestamp or datetime.now()) + .astimezone() + .isoformat(timespec="seconds"), COL_STEP: str(step), COL_STATUS: str(status), } diff --git a/tests/cli/test_record.py b/tests/cli/test_record.py index c741cd7..d5cb019 100644 --- a/tests/cli/test_record.py +++ b/tests/cli/test_record.py @@ -1,5 +1,5 @@ from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import ANY, MagicMock import pandas as pd import pytest @@ -49,7 +49,11 @@ def test_start(self, runner: CliRunner, mock_record: MagicMock, dpath_root: Path ) assert "Saved answers" not in result.stdout mock_record.assert_called_once_with( - dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} + dpath_root.resolve(), + Step.IDP_EXTRACTION, + Status.START, + answers={}, + timestamp=ANY, ) @pytest.mark.parametrize( @@ -83,6 +87,7 @@ def test_end_other( Step.FEDBIOMED_NODE_PART_1, Status.END, answers={QUESTION_NOTES: notes}, + timestamp=ANY, ) @pytest.mark.parametrize( @@ -118,6 +123,7 @@ def test_end_data_prep( QUESTION_TIME_WITHOUT_TOOLS: "1-4h", QUESTION_NOTES: "went fine", }, + timestamp=ANY, ) def test_end_data_prep_invalid_input_reprompts( @@ -138,6 +144,31 @@ def test_end_data_prep_invalid_input_reprompts( QUESTION_TIME_WITHOUT_TOOLS: ">7d", QUESTION_NOTES: "", }, + timestamp=ANY, + ) + + def test_timestamp_before_prompts( + self, + runner: CliRunner, + mock_record: MagicMock, + dpath_root: Path, + mocker: MockerFixture, + ): + manager = MagicMock() + mock_datetime = mocker.patch("brainflex.cli.record.datetime") + mock_ask = mocker.patch("brainflex.cli.record.Prompt.ask", return_value="") + manager.attach_mock(mock_datetime.now, "now") + manager.attach_mock(mock_ask, "ask") + result = runner.invoke( + app, + ["record", "fedbiomed_node_part_1", "end", "--project", str(dpath_root)], + ) + assert result.exit_code == 0 + names = [name for name, *_ in manager.mock_calls] + assert names.index("now") < names.index("ask") + assert ( + mock_record.call_args.kwargs["timestamp"] + is mock_datetime.now.return_value.astimezone.return_value ) def test_default_path( @@ -151,7 +182,11 @@ def test_default_path( result = runner.invoke(app, ["record", "idp_extraction", "start"]) assert result.exit_code == 0 mock_record.assert_called_once_with( - dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} + dpath_root.resolve(), + Step.IDP_EXTRACTION, + Status.START, + answers={}, + timestamp=ANY, ) @pytest.mark.parametrize("explicit", [True, False]) @@ -176,7 +211,11 @@ def test_subdirectory( ) assert result.exit_code == 0 mock_record.assert_called_once_with( - dpath_root.resolve(), Step.IDP_EXTRACTION, Status.START, answers={} + dpath_root.resolve(), + Step.IDP_EXTRACTION, + Status.START, + answers={}, + timestamp=ANY, ) @pytest.mark.parametrize( diff --git a/tests/core/test_record.py b/tests/core/test_record.py index f63b7be..e5f9a60 100644 --- a/tests/core/test_record.py +++ b/tests/core/test_record.py @@ -1,5 +1,5 @@ import warnings -from datetime import datetime +from datetime import datetime, timedelta, timezone from pathlib import Path import pandas as pd @@ -80,6 +80,13 @@ def test_start(self, root: Path, fpath_timestamps: Path, fpath_survey: Path): assert datetime.fromisoformat(row["timestamp"]).tzinfo is not None assert not fpath_survey.exists() + def test_timestamp(self, root: Path, fpath_timestamps: Path): + timestamp = datetime(2026, 1, 2, 3, 4, 5, tzinfo=timezone(timedelta(hours=-5))) + record(root, Step.IDP_EXTRACTION, Status.START, timestamp=timestamp) + assert _read(fpath_timestamps)["timestamp"].iloc[0] == ( + "2026-01-02T03:04:05-05:00" + ) + def test_end_data_prep( self, root: Path, fpath_timestamps: Path, fpath_survey: Path ): From d7f8be323e63eb66147622e4967afa978d8b1be4 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 15:56:03 -0500 Subject: [PATCH 15/16] fix tests --- tests/cli/test_initial_survey.py | 2 +- tests/core/test_initial_survey.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cli/test_initial_survey.py b/tests/cli/test_initial_survey.py index ed4469e..569f161 100644 --- a/tests/cli/test_initial_survey.py +++ b/tests/cli/test_initial_survey.py @@ -119,7 +119,7 @@ def test_end_to_end(self, runner: CliRunner, dpath_root: Path, fpath_survey: Pat assert result.exit_code == 0 assert fpath_survey.read_text().splitlines() == [ "question\tanswer", - "How familiar are you with Nipoppy?\t1 (Never heard of it before)", + "How familiar are you with Nipoppy?\t1 (I have never heard of it before this project)", "How familiar are you with Neurobagel?\t3 (I have used but only with example/toy data)", "How familiar are you with Fed-BioMed?\t5 (I use it regularly / It is one of my go-to tools)", ] diff --git a/tests/core/test_initial_survey.py b/tests/core/test_initial_survey.py index 254c75c..06e91e3 100644 --- a/tests/core/test_initial_survey.py +++ b/tests/core/test_initial_survey.py @@ -56,7 +56,7 @@ def test_creates_file(self, root: Path, fpath_survey: Path): "How familiar are you with Fed-BioMed?", ] assert list(df["answer"]) == [ - "1 (Never heard of it before)", + "1 (I have never heard of it before this project)", "3 (I have used but only with example/toy data)", "5 (I use it regularly / It is one of my go-to tools)", ] @@ -67,7 +67,7 @@ def test_question_order(self, root: Path, fpath_survey: Path): df = _read(fpath_survey) assert list(df["question"]) == list(INITIAL_SURVEY_QUESTIONS) assert list(df["answer"]) == [ - "1 (Never heard of it before)", + "1 (I have never heard of it before this project)", "3 (I have used but only with example/toy data)", "5 (I use it regularly / It is one of my go-to tools)", ] From 5705123ef7d3947c2ad2d415f4c7af750e10bcf1 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Wed, 4 Nov 2026 16:03:05 -0500 Subject: [PATCH 16/16] attempt to fix test --- src/brainflex/core/record.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/brainflex/core/record.py b/src/brainflex/core/record.py index 0db0b75..861e35a 100644 --- a/src/brainflex/core/record.py +++ b/src/brainflex/core/record.py @@ -144,10 +144,11 @@ def record( dpath = root / BRAINFLEX_DIRNAME fpath_timestamps = dpath / TIMESTAMPS_FILENAME + timestamp = timestamp or datetime.now() + if timestamp.tzinfo is None: + timestamp = timestamp.astimezone() # naive -> interpret as local time row = { - COL_TIMESTAMP: (timestamp or datetime.now()) - .astimezone() - .isoformat(timespec="seconds"), + COL_TIMESTAMP: timestamp.isoformat(timespec="seconds"), COL_STEP: str(step), COL_STATUS: str(status), }