-
Notifications
You must be signed in to change notification settings - Fork 70
FXC-1530: add total current normalization to UniformCurrentSource #3236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
|
3a31f35 to
2348ec5
Compare
weiliangjin2021
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment on naming.
bcb6337 to
13f4669
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
2c8d636 to
a937e89
Compare
|
I added a validation warning about changing the default behavior in the future. Is this how we typically do this? |
momchil-flex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, generally we've also been trying to say in which version exactly we would change the behavior. But if you're not sure (e.g. if we might end up changing it in rf but not in photonics) could leave as is.
So you preemptively updated tests to the new behavior?
I had to change a lot of tests since they were emitting the warning, and then the |
a937e89 to
0585331
Compare
0585331 to
b9c8e48
Compare
Note
Medium Risk
Touches a core source model and adds new validation-time warnings plus a new normalization mode; while defaults are preserved, behavior and user-visible logs change and could affect downstream workflows relying on implicit defaults.
Overview
Adds a new
UniformCurrentSource.current_amplitude_definitionfield ("density"default, or"total") to support size-independent total-current injection, and updates theUniformCurrentSourcedocs/example accordingly.Introduces a post-init validation warning when
current_amplitude_definitionis not explicitly set, signaling a future default change from"density"to"total", and propagates the new field into JSON schemas and key call sites (e.g., rectangular lumped port source creation) plus broad test updates/new warning coverage.Written by Cursor Bugbot for commit b9c8e48. This will update automatically on new commits. Configure here.