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.
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
Corner cases catches #70
Corner cases catches #70
Changes from 8 commits
df7d76c
5cc4f87
1c30a12
55c64c3
80399c7
b7955f4
a794de1
543a801
e16cb6b
f983a81
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is this now a
Vector
rather than aSet
?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.
because before it was incorrectly using the ref buses positions and not the ref buses values.
Check warning on line 324 in src/common.jl
Codecov / codecov/patch
src/common.jl#L324
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.
Ideally we'd get a test case that demonstrates what breaks without this new layer of indirection — if too complicated to do now but potentially feasible in the future, add a
# TODO
or GitHub IssueThere 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.
Would it be possible to get a test case of the element of the public interface that depends on this that fails without this change and passes with it? Nothing fails when I comment this line out.
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.
or a
# TODO
or issue if this is too complicated to do on the current timeframeThere 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.
Codecov concurs and points out a few other untested regions
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.
I will create an issue to implement the test. This is a corner case catch.
Check warning on line 107 in src/network_radial_reduction.jl
Codecov / codecov/patch
src/network_radial_reduction.jl#L105-L107
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.
I don't really understand why we're doing this, but that may be due to my lack of familiarity with PNM
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.
We never want to reduce the reference buses. It creates issues depending on the model once we remove tha bus that is meant to absorb all the power differences.
Check warning on line 149 in src/network_radial_reduction.jl
Codecov / codecov/patch
src/network_radial_reduction.jl#L149
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.
All the other existing
assign_reference_buses
calls have been edited to add abus_ax_ref
— this one doesn't need one?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.
in some cases we already have the references buses so there is no need to recalculate them based on the ref_ax and location.