Skip to content

Cross-correlation: raise ValueError when fewer than two signal regions are present and surface error to frontend UI#284

Merged
pauladkisson merged 6 commits intomainfrom
copilot/fix-cross-correlation-error
Apr 16, 2026
Merged

Cross-correlation: raise ValueError when fewer than two signal regions are present and surface error to frontend UI#284
pauladkisson merged 6 commits intomainfrom
copilot/fix-cross-correlation-error

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 16, 2026

  • Raise ValueError in execute_compute_cross_correlation when fewer than two signal regions are present (single-region and zero-region messages)
  • Update getCorrCombinations to return single region name instead of empty list
  • Surface error messages to Panel frontend UI via PB_ERROR_FILE mechanism and pn.state.notifications.error
  • Add/update unit tests (psth_utils, psth orchestration, progress)
  • Update CHANGELOG.md
  • Mark readPBIncrementValues tests with @pytest.mark.progress_bar; skip on Windows in CI; add # pragma: no cover to readPBIncrementValues (see issue Refactor progress bar to remove file-based backend #286)

…ignal regions

Agent-Logs-Url: https://github.com/LernerLab/GuPPy/sessions/5e225688-eb0d-40ff-be9d-78714b739f3e

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix cross-correlation to raise error with one region Cross-correlation: raise ValueError when fewer than two signal regions are present Apr 16, 2026
Copilot AI requested a review from pauladkisson April 16, 2026 16:25
Copy link
Copy Markdown
Collaborator

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

This actually looks pretty good on the backend side, but I would like this error message to surface to the frontend so that users know that something has gone wrong when they click step five and they don't have to go digging into the terminal.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 16, 2026

This actually looks pretty good on the backend side, but I would like this error message to surface to the frontend so that users know that something has gone wrong when they click step five and they ...

Done in commit 797e3a0. The error message is now surfaced to the Panel UI via a toast notification:

  • Added PB_ERROR_FILE (~/pbError.txt) — each subprocess main() (psth, preprocess, read_raw_data) writes the exception message there before writing the -1 failure sentinel to the progress file (order guarantees the file is present when the frontend reads it).
  • readPBIncrementValues now reads and returns the error message from that file when it detects -1, then cleans it up.
  • home.py enables pn.extension(notifications=True) and calls pn.state.notifications.error(error_msg, duration=0) after each step button click if an error message was returned — so users see a persistent red toast notification directly in the UI without needing to check the terminal.

Copilot AI changed the title Cross-correlation: raise ValueError when fewer than two signal regions are present Cross-correlation: raise ValueError when fewer than two signal regions are present and surface error to frontend UI Apr 16, 2026
Copilot AI requested a review from pauladkisson April 16, 2026 16:41
Copy link
Copy Markdown
Collaborator

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

This looks amazing. The error pop-up is beautiful.

@pauladkisson pauladkisson marked this pull request as ready for review April 16, 2026 17:34
@pauladkisson
Copy link
Copy Markdown
Collaborator

@copilot please update the change log.

Agent-Logs-Url: https://github.com/LernerLab/GuPPy/sessions/3aa24ae1-7be2-479d-8785-6ff1dff16708

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 16, 2026

@copilot please update the change log.

Done in commit d7eff42 — added an entry under Fixes in the v2.0.0-alpha5 section of CHANGELOG.md.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 40.00000% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/guppy/orchestration/home.py 10.00% 9 Missing ⚠️
src/guppy/orchestration/preprocess.py 33.33% 2 Missing ⚠️
src/guppy/orchestration/psth.py 71.42% 2 Missing ⚠️
src/guppy/orchestration/read_raw_data.py 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

Unfortunately, these tests fail on Windows due to threading and file lock issues. I think the cleanest solution to this problem is to mark those tests as progress bar tests and/or something like that. I think progress bar tests, and then skip them in the CI so that they don't run on Windows, but they can be run locally. Add a pragma: no cover to the read PB increment values function, along with a comment explaining why it cannot be tested with a reference to this issue.

#286

…readPBIncrementValues

Agent-Logs-Url: https://github.com/LernerLab/GuPPy/sessions/7159dbbd-a7a0-477a-8676-107700b03ffc

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 16, 2026

Unfortunately, these tests fail on Windows due to threading and file lock issues. I think the cleanest solution to this problem is to mark those tests as progress bar tests and/or something like that....

Done in commit 3363c4e:

  • Added a progress_bar marker to pyproject.toml (with a description noting it's skipped on Windows in CI due to issue Refactor progress bar to remove file-based backend #286).
  • Decorated both readPBIncrementValues tests in test_progress.py with @pytest.mark.progress_bar.
  • Updated .github/workflows/run-tests.yml to append and not progress_bar to the marker expression when runner.os == 'Windows'.
  • Added # pragma: no cover to readPBIncrementValues in progress.py with a docstring comment explaining why (threading/file-lock behaviour unreliable on Windows in CI) and referencing issue Refactor progress bar to remove file-based backend #286.

Copilot AI requested a review from pauladkisson April 16, 2026 19:25
@pauladkisson pauladkisson enabled auto-merge April 16, 2026 21:31
@pauladkisson pauladkisson disabled auto-merge April 16, 2026 21:46
@pauladkisson pauladkisson merged commit a8db92b into main Apr 16, 2026
10 of 11 checks passed
@pauladkisson pauladkisson deleted the copilot/fix-cross-correlation-error branch April 16, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cross-correlation: error when enabled with only one region instead of silently ignoring

2 participants