Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors core buck/boost converter calculations into reusable calculate_parameters methods, adds range utilities, and introduces an FCML power path example with scaling support.
- Centralize component sizing logic into
BuckConverterPowerPath.ValuesandBoostConverterPowerPath.Valuesand updategenerateto use them - Extend
Range/RangeExprwithhullandintersectand cover them with new tests - Add
FcmlPowerPathexample and update Simon/Fcml netlists to reference it
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examples/test_fcml.py | Added FcmlPowerPath example for inductor scaling |
| examples/Simon/Simon.ref.net | Updated inductor value and part reference |
| examples/Simon/Simon.net | Updated inductor value and part reference |
| examples/Fcml/Fcml.ref.net | Pointed part source to FcmlPowerPath |
| examples/Fcml/Fcml.net | Pointed part source to FcmlPowerPath |
| edg/core/test_range.py | Renamed and expanded tests for intersects and intersect |
| edg/core/Range.py | Implemented hull and intersect methods |
| edg/core/ConstraintExpr.py | Unified arithmetic operator handling with _CASTABLE_TYPES |
| edg/abstract_parts/test_switching_converters.py | New calculation tests for buck/boost converters |
| edg/abstract_parts/AbstractPowerConverters.py | Introduced Values NamedTuples and calculate_parameters; refactored generate |
Comments suppressed due to low confidence (4)
edg/core/test_range.py:59
- [nitpick] There are two test methods:
test_intersectsandtest_intersect, which may confuse their intent. Consider renaming them totest_intersects_booleanandtest_intersect_resultfor clarity.
def test_intersect(self):
examples/test_fcml.py:177
- [nitpick] Update or remove this TODO in the class docstring—either note the intended duplication or refactor to clarify why the code is duplicated.
TODO: this basically completely duplicates BuckConverterPowerPath, but adds a scaling factor that doesn't exist there
edg/abstract_parts/AbstractPowerConverters.py:207
- [nitpick] Add a docstring above the
ValuesNamedTuple to explain each field and its units, improving discoverability for API consumers.
class Values(NamedTuple):
examples/test_fcml.py:175
- Consider adding unit tests for
FcmlPowerPath.calculate_parametersand its scaling factor logic to verify the new behavior.
class FcmlPowerPath(InternalSubcircuit, GeneratorBlock):
edg/core/Range.py
Outdated
|
|
||
| def intersect(self, other: 'Range') -> 'Range': | ||
| # TODO make behavior more consistent w/ compiler and returning empty that props as a unit | ||
| assert self.intersects(other), "Cannot intersect ranges that do not intersect" |
There was a problem hiding this comment.
Using an assert here will be skipped if Python is run with optimizations. Consider raising a ValueError for invalid intersection to ensure the check always runs.
Suggested change
| assert self.intersects(other), "Cannot intersect ranges that do not intersect" | |
| if not self.intersects(other): | |
| raise ValueError("Cannot intersect ranges that do not intersect") |
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.
This makes the core component calculations more unit-testable and unifies the buck-boost converter (as well as allowing other variations-on-a-theme converters like forward-buck-reverse-boost as with some NVDC battery chargers with OTG with less code duplication)
Changes the boost inductor calculation to avoid double-counting output tolerance. The inductance formula appears monotonic.
Core infrastructural changes:
TODO:
Follow-on PRs will add coupled current/inductance support (#405) and the MP2722 converter which this is building towards.