Use the same stable seed logic for SRF generation as HF#96
Use the same stable seed logic for SRF generation as HF#96lispandfound wants to merge 7 commits intopegasusfrom
Conversation
…ation as HF generation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the seed generation logic by centralizing the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly extracts the stable_hash function into a shared utility to ensure consistent seed generation logic. However, the changes introduce issues with testing. An existing test is broken by the refactoring, and the new logic in realisation-to-srf.py lacks test coverage. My review provides specific comments to address these critical and medium severity testing gaps.
I am having trouble creating individual review comments. Click here to see my feedback.
workflow/scripts/hf_sim.py (250-273)
Removing stable_hash from this module breaks the test test_stable_hash in tests/test_hf.py, which will now fail with an AttributeError. To resolve this, the test needs to be moved and updated.
Please move test_stable_hash from tests/test_hf.py to tests/test_utils.py and adapt it to test utils.stable_hash. This will likely involve adding imports for hypothesis to tests/test_utils.py.
workflow/scripts/realisation_to_srf.py (549-552)
This change introduces new seed generation logic, but it appears to be untested. To ensure the correctness and maintainability of this new feature, please add a unit test for generate_fault_srf that verifies the seed parameter passed to _build_genslip_command is calculated correctly using the stable hash of the fault name.
There was a problem hiding this comment.
Pull request overview
This PR extracts the HF simulation’s stable hashing logic into workflow.utils and reuses it in SRF generation so that per-segment seeds are derived deterministically from a root seed and a segment/station name, making results invariant to generation order.
Changes:
- Add
utils.stable_hash()(blake2b-based) as a shared utility. - Update
realisation-to-srfto derive a per-segment genslip seed using the stable hash of the segment name. - Update HF station seed derivation to use
utils.stable_hash()instead of a local implementation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
workflow/utils.py |
Adds shared stable_hash() utility for stable, order-invariant name hashing. |
workflow/scripts/realisation_to_srf.py |
Uses stable_hash(name) to derive per-segment genslip seeds from the root seed. |
workflow/scripts/hf_sim.py |
Removes local stable_hash and switches station seed hashing to utils.stable_hash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Extracts the hf sim
stable_hashutility intoworkflow.utilsso thatrealisation-to-srfcan use it to generate per-segment seeds from the root seed using the station name as extra entropy. Like the hf simulation, this ensures seeds are stable, and invariant to the order that segment SRFs are generated in. Added a few property tests in case we change the implementation in the future that should catch anything the code relies on namely:sizeargument.Any function satisfying the above should be fine.