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
Implementation of feature 720 asym_line #762
base: main
Are you sure you want to change the base?
Implementation of feature 720 asym_line #762
Changes from all commits
0cde92c
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.
with this being in the full namespace of this project, i'm a little bit hesitant to add single-letter names here. let's think about whether this is the way to go or whether it's preferable to just use
std::numbers::e
where it's used (since it's not that common in the code base anyways). alternatively, we can put them all in apower_grid_model::numbers
namespace and expose only the longer ones outside.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 think we explicitly check input parameters in any of the other components. only things like ID references to other components are checked. I would just consider this UB (undefined behavior; in this case in particular unspecified behavior). The data validator in Python can check that it is supported, but the core should not.
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'd go for something more explicit. It's very easy to make a mistake in the order.
A proposal is the one below (BEWARE: this changes the order, so usage needs to be adjusted)
alternatively, you can provide 2
Vector<T>
as input for readabilityThere 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.
idem to my comment on Tensor3