Optimize and harden clusterless position decoding - #3
Open
CommanderPho wants to merge 1 commit into
Open
Conversation
- Introduced a cache in `_perform_clusterless_position_decoding_computation` to prevent computing multiunits multiple times for the same time bounds, significantly speeding up the pipeline when running both 1D and 2D decoders. - Removed an unnecessary dataframe copy operation before creating multiunits. - Added safety checks to verify validity of `pos_df` and time data before decoding. - Added defensive `try-except` blocks around decoder initialization to ensure errors won't crash the entire computation pipeline. Co-authored-by: CommanderPho <962210+CommanderPho@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds robustness checks, logging, and multiunit caching to clusterless position decoding, ensuring decoders are only built when valid positional data exist and reusing expensive multiunit computations across 1D/2D placefield decoders. Sequence diagram for clusterless position decoding with validation and multiunit cachingsequenceDiagram
participant ComputationResult
participant perform_clusterless_position_decoding as _perform_clusterless_position_decoding_computation
participant build_decoder_for_pf as _build_decoder_for_pf
participant MultiunitCache as _multiunits_cache
participant Session as sess
participant Decoder as ClusterlessRTCPositionDecoder
participant Logging as logging
ComputationResult->>perform_clusterless_position_decoding: call
perform_clusterless_position_decoding->>build_decoder_for_pf: _build_decoder_for_pf(pf1D)
perform_clusterless_position_decoding->>build_decoder_for_pf: _build_decoder_for_pf(pf2D)
rect rgb(230,230,250)
build_decoder_for_pf-->>build_decoder_for_pf: [pf is None or pos_df invalid]
build_decoder_for_pf->>Logging: logging.warning
build_decoder_for_pf-->>perform_clusterless_position_decoding: return None
end
rect rgb(220,255,220)
build_decoder_for_pf-->>build_decoder_for_pf: [pos_df has valid time column]
build_decoder_for_pf-->>build_decoder_for_pf: compute t_start, t_end
alt multiunits is not None
build_decoder_for_pf->>Session: build_multiunits_from_array(multiunits, rtc_time)
else multiunits is None
alt cache hit
build_decoder_for_pf->>MultiunitCache: get (t_start, t_end)
else cache miss
build_decoder_for_pf->>Session: build_multiunits_from_session(sess, sampling_frequency_hz, t_start, t_end, spikes_df)
build_decoder_for_pf->>MultiunitCache: set (t_start, t_end)
end
end
build_decoder_for_pf->>Decoder: ClusterlessRTCPositionDecoder(...)
build_decoder_for_pf->>Decoder: compute_all()
build_decoder_for_pf-->>perform_clusterless_position_decoding: return Decoder
end
rect rgb(255,230,230)
build_decoder_for_pf-->>build_decoder_for_pf: [exception]
build_decoder_for_pf->>Logging: logging.error
build_decoder_for_pf-->>perform_clusterless_position_decoding: return None
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_multiunits_cachecurrently keys on rawt_start/t_endfloats, which may differ due to floating point precision; consider normalizing or discretizing these values (or keying by a more stable identifier like the underlying time index range) to avoid ineffective caching. - The broad
except Exception as ein_build_decoder_for_pfsilently converts all decoder build failures intoNone; consider either narrowing the exception types or re-raising after logging to avoid masking unexpected errors. - Instead of importing
logginginside_perform_clusterless_position_decoding_computation, consider using a module-level logger (e.g.,logger = logging.getLogger(__name__)) so that log messages can be more easily controlled and are consistent with typical logging patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_multiunits_cache` currently keys on raw `t_start`/`t_end` floats, which may differ due to floating point precision; consider normalizing or discretizing these values (or keying by a more stable identifier like the underlying time index range) to avoid ineffective caching.
- The broad `except Exception as e` in `_build_decoder_for_pf` silently converts all decoder build failures into `None`; consider either narrowing the exception types or re-raising after logging to avoid masking unexpected errors.
- Instead of importing `logging` inside `_perform_clusterless_position_decoding_computation`, consider using a module-level logger (e.g., `logger = logging.getLogger(__name__)`) so that log messages can be more easily controlled and are consistent with typical logging patterns.
## Individual Comments
### Comment 1
<location path="src/pyphoplacecellanalysis/General/Pipeline/Stages/ComputationFunctions/DefaultComputationFunctions.py" line_range="128" />
<code_context>
+ if cache_key in _multiunits_cache:
+ pf_multiunits, pf_rtc_time = _multiunits_cache[cache_key]
+ else:
+ pf_multiunits, pf_rtc_time = build_multiunits_from_session(sess, clusterless_params.clusterless_sampling_frequency_hz, t_start, t_end, spikes_df=pf.filtered_spikes_df)
+ _multiunits_cache[cache_key] = (pf_multiunits, pf_rtc_time)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping the `.copy()` on `pf.filtered_spikes_df` may introduce unexpected mutation of shared data.
This used to pass `pf.filtered_spikes_df.copy()`, but now passes the original DataFrame. If `build_multiunits_from_session` mutates `spikes_df`, that will also mutate `filtered_spikes_df` and any other code using it. Please either restore the `.copy()` or confirm that `build_multiunits_from_session` treats `spikes_df` as read-only.
</issue_to_address>
### Comment 2
<location path="src/pyphoplacecellanalysis/General/Pipeline/Stages/ComputationFunctions/DefaultComputationFunctions.py" line_range="134-136" />
<code_context>
+ decoder = ClusterlessRTCPositionDecoder(pf=pf, sampling_frequency_hz=clusterless_params.clusterless_sampling_frequency_hz, multiunits=pf_multiunits, rtc_time=pf_rtc_time, clusterless_params=clusterless_params, setup_on_init=True, post_load_on_init=False, debug_print=False)
+ decoder.compute_all()
+ return decoder
+ except Exception as e:
+ logging.error(f"Error building clusterless decoder: {e}")
+ return None
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching all exceptions and only logging the message may make debugging decoder failures harder.
The broad `except Exception` likely keeps the pipeline running, but the current `logging.error(f"...{e}")` drops the traceback and context needed to debug failures. Prefer `logging.exception("Error building clusterless decoder")` so the full stack trace is logged, and consider selectively re-raising or handling specific exception types for critical failures.
```suggestion
except Exception:
logging.exception("Error building clusterless decoder")
return None
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if cache_key in _multiunits_cache: | ||
| pf_multiunits, pf_rtc_time = _multiunits_cache[cache_key] | ||
| else: | ||
| pf_multiunits, pf_rtc_time = build_multiunits_from_session(sess, clusterless_params.clusterless_sampling_frequency_hz, t_start, t_end, spikes_df=pf.filtered_spikes_df) |
There was a problem hiding this comment.
issue (bug_risk): Dropping the .copy() on pf.filtered_spikes_df may introduce unexpected mutation of shared data.
This used to pass pf.filtered_spikes_df.copy(), but now passes the original DataFrame. If build_multiunits_from_session mutates spikes_df, that will also mutate filtered_spikes_df and any other code using it. Please either restore the .copy() or confirm that build_multiunits_from_session treats spikes_df as read-only.
Comment on lines
+134
to
+136
| except Exception as e: | ||
| logging.error(f"Error building clusterless decoder: {e}") | ||
| return None |
There was a problem hiding this comment.
suggestion (bug_risk): Catching all exceptions and only logging the message may make debugging decoder failures harder.
The broad except Exception likely keeps the pipeline running, but the current logging.error(f"...{e}") drops the traceback and context needed to debug failures. Prefer logging.exception("Error building clusterless decoder") so the full stack trace is logged, and consider selectively re-raising or handling specific exception types for critical failures.
Suggested change
| except Exception as e: | |
| logging.error(f"Error building clusterless decoder: {e}") | |
| return None | |
| except Exception: | |
| logging.exception("Error building clusterless decoder") | |
| return None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optimize performance and robustness of
_perform_clusterless_position_decoding_computationPR created automatically by Jules for task 10468796758105246808 started by @CommanderPho
Summary by Sourcery
Optimize clusterless position decoding by caching multiunit computations and hardening decoder construction against missing or invalid positional data.
Bug Fixes:
Enhancements: