-
Notifications
You must be signed in to change notification settings - Fork 52
fix elmer #665
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the Elmer capacitive simulation helper to use the new prism mesh generation signature and the updated component ports API when deriving ground layers, removing now-invalid parameters and attributes. Class diagram for updated Elmer capacitance helper dependenciesclassDiagram
class Component {
+ports list~Port~
}
class Port {
+layer int
}
class LayerStack {
+layers dict~str,Layer~
}
class Layer {
+layer int
}
class MeshwellPrismGenerator {
+get_meshwell_prisms(component Component, layer_stack LayerStack) list~Prism~
}
class Prism {
}
class ElmerCapacitanceHelper {
+run_capacitive_simulation_elmer(component Component, layer_stack LayerStack, simulation_folder Path, filename str, n_processes int) None
}
ElmerCapacitanceHelper --> Component
ElmerCapacitanceHelper --> LayerStack
ElmerCapacitanceHelper --> MeshwellPrismGenerator
Component "1" --> "many" Port
LayerStack "1" --> "many" Layer
MeshwellPrismGenerator --> Prism
Port --> Layer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- The
get_meshwell_prismscall no longer passesfilenameorn_threads; if the mesh output location or parallelism is still important for performance or reproducibility, consider explicitly configuring these via the updated API rather than relying on defaults. - Switching from
component.get_ports()tocomponent.portschanges the dependency on the component API; ensurecomponent.portsis guaranteed to return the same type/structure (e.g., ordered, filtered to relevant ports) or add a small adapter if normalization is needed here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `get_meshwell_prisms` call no longer passes `filename` or `n_threads`; if the mesh output location or parallelism is still important for performance or reproducibility, consider explicitly configuring these via the updated API rather than relying on defaults.
- Switching from `component.get_ports()` to `component.ports` changes the dependency on the component API; ensure `component.ports` is guaranteed to return the same type/structure (e.g., ordered, filtered to relevant ports) or add a small adapter if normalization is needed here.
## Individual Comments
### Comment 1
<location> `gplugins/elmer/get_capacitance.py:263` </location>
<code_context>
ground_layers = {
next(k for k, v in layer_stack.layers.items() if v.layer == port.layer)
- for port in component.get_ports()
+ for port in component.ports
} # ports allowed only on metal
metal_surfaces = [
</code_context>
<issue_to_address>
**issue (bug_risk):** Iterating over `component.ports` likely yields keys, not Port objects, which will break `port.layer` access.
If `component.ports` is a mapping (name -> Port), iterating over it returns names, so `port.layer` will raise at runtime. To match the original `get_ports()` behavior of yielding Port objects, iterate over the values instead, e.g. `for port in component.ports.values()` (or the equivalent for this component type).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Align Elmer capacitive simulation helper with updated component and meshing APIs to resolve runtime issues.
Bug Fixes: