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
HybridACPowerFlow #97
base: main
Are you sure you want to change the base?
HybridACPowerFlow #97
Changes from 24 commits
11d5c48
dbdcec1
5168045
6ad51aa
b87ec7d
8fc3e4f
bf3474a
bd0d454
5a6c66c
9a9fc38
479c33a
61a4a11
95bd687
cf58844
20ac49c
8458a23
103eb04
aaf9480
d51813e
e22ae07
ddae98e
4c34a7c
1e39bb5
87c4a09
2eda04d
2d8241a
445bd9b
fa4e013
a082bf3
e1bbbdd
5b84e13
18becd9
d9848b6
cc8e3ed
3d59c5f
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.
Is accessing the
_symbolic
and_numeric
fields ofK
part of KLU's public interface? If not (and my guess is not), find a way to avoidThere 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.
This was a subject of discussion with @rbolgaryn. We're doing things this way (directly calling C wrappers, using private fields) for performance reasons: KLU's public interface was not written in a non-allocating manner. That being said, I have not actually measured the performance difference.
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.
Maybe @jd-lara and @daniel-thom want to weigh in here but I'd be veeery wary of doing this. I guess you could check the Git history to find out how stable in practice some of these things are, but typically when a field starts with an underscore it's to explicitly signal that it should not be relied upon externally….
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.
Alternatively, how hard would it be to write our own wrapper to replace KLU.jl?
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.
This is how it looks in the KLU.jl package:
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 general, I'm not fully sold on the use of
C_NULL
as a null value except in cases where a (public) interface with a library requires it.rf_numeric
is already aUnion
; does it really impact performance to put the option ofNothing
in there? (If it really does, I could get on board with this.)