Rnc/testing merge in from cocos#11
Open
chandrarn wants to merge 24 commits into
Open
Conversation
…ality we only need one helicity value in the end, and it should be pulled from Sgn[Ip, B_t].
… gracefully and consistently with helicity wrapping direction. Issue likely arrises from COCOS reasons
…solute value of m/n is probably constant due to fixed helicity, the sign of both can change together, to account for rotation direction. This appears to fix the issue on shot 1120906030. Interestingly, it still predicts negative rotation for mode-conversion flow shot 1110316031
…nor value during filament tracking: limits now use r_magx and r_bdry.max from gEQDSK file
…sde search options, should be more stable)
…o close to outer boundary b, move the boundary backwards
Testing merge from COCOS updates/Zander's branch into my branch. My branch includes updates for detecting R_m, a_m of target rational surface as eta evolves. New branch changes helicity interpretations and probe naming conventions. Naming convention changes accepted for now.
…ed on a per-filament basis, instead of a per-mesh basis: breaking changes in the current OFT branch now no longer allow the number of resistivity values to be inequal (including, now, more than) the number of conducting structure groups. Updated test_equilibrium_field.py to skip TVC, DIII-D, as well as C-Mod tests if their respective gEQDSK files are missing
…ed on a per-filament basis, instead of a per-mesh basis: breaking changes in the current OFT branch now no longer allow the number of resistivity values to be inequal (including, now, more than) the number of conducting structure groups. Updated test_equilibrium_field.py to skip TVC, DIII-D, as well as C-Mod tests if their respective gEQDSK files are missing ammending change: removing uv.lock and pyproject.toml from commit due to breaking changes in the way that we handle importing OFT
…/SynthWave into rnc/Testing_Merge_In_From_COCOS Prepping for PR
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates filament root solving for equilibrium tracing and adjusts tests for missing machine-specific data and ThinCurr eta semantics.
Changes:
- Adds an adaptive root-solving helper in filament tracing.
- Skips EQDSK-dependent tests when required files are missing.
- Updates a ThinCurr test to pass a single resistivity value for the conducting structure group.
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
synthwave/magnetic_geometry/filaments.py |
Adds adaptive root solving and updates equilibrium tracing root calls. |
synthwave/tests/test_equilibrium_field.py |
Centralizes EQDSK paths and skips C-Mod/DIII-D tests when files are unavailable. |
synthwave/tests/test_filaments.py |
Skips C-Mod-dependent filament tests when the EQDSK file is unavailable. |
synthwave/tests/test_thincurr.py |
Updates ThinCurr eta input in the direct/frequency response comparison test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Acceptable fix Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ndary Agent-Logs-Url: https://github.com/MIT-PSFC/SynthWave/sessions/c133acf8-23e4-469f-a92b-5f11f19f7717 Co-authored-by: chandrarn <8441592+chandrarn@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
chandrarn
commented
May 15, 2026
Collaborator
Author
chandrarn
left a comment
chandrarn
left a comment
There was a problem hiding this comment.
review of copilot suggestions complete
…ity sign, and thus wouldn't change direction under the parallel or antiparallel transformation
…ity sign, and thus wouldn't change direction under the parallel or antiparallel transformation
ZanderKeith
reviewed
May 22, 2026
Collaborator
There was a problem hiding this comment.
Why is this here? If it's a g file used in tests it should be moved to tests/input_data
However, I don't see it being referenced anywhere.
ZanderKeith
requested changes
May 22, 2026
Collaborator
There was a problem hiding this comment.
I do not see this file being referenced anywhere. Is it necessary?
Collaborator
There was a problem hiding this comment.
20 MB file that isn't part of this repo should not be getting tracked in git history.
Collaborator
There was a problem hiding this comment.
New method looks ok to me, but I'd like to see a test that demonstrates it actually has improved convergence. Do you have an instance where the previous method failed and this one works?
| ) | ||
|
|
||
| # Test one frequency | ||
| # Disable HODLR compression to improve convergence |
Collaborator
There was a problem hiding this comment.
Is HODLR compression on by default?
Also only the comment changed in this file, are we really doing anything differently now?
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
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.
Physics changes:
synthwave/magnetic_geometry/filaments._solve_root_with_adaptive_bracket()to catch edge cases better than the Newton solver for R_maj, a_min vs eta. The goal here is to better handle[self.eq_field.eqdsk.rbdry.max() - self.eq_field.eqdsk.rmagx + 0.2]. It falls back on the Newton method viaroot_scalarif the minimized value cannot be found.Pytest changes:
synthwave/tests/test_equilibrium_field.pyskips machine-specific individual tests for cases where the respective gEQDSK file is not found.synthwave/tests/test_thincurr.pymodified to change the interpretation of eta as a per-filament resistivity to a per-conducting structure mesh group resistivity. The former interpretation was incorrect, but did not break the code. However, the current dev branch update of OFT removes this leniency: the number of conducting structure "groups" in the .h5 file now must exactly match the number of eta values. Previously, although the interpretation of eta was incorrect, ThinCurr just grabbed the n eta values it needed, disregarding the rest. The correct does not introducing breaking changes for the older OFT copy: there is now the correct number of eta values, just not more than that.Note:
uv.lockandpyproject.tomlare not included in this PR, so as to not mess with the existing handling of the OFT import. I will keep my versions changed independently on my branch (the.whlimport strategy)