-
Notifications
You must be signed in to change notification settings - Fork 0
Initial PR #1
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
Initial PR #1
Conversation
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.
Great work, @hfytr. Please see my initial round of comments
[ Info: Running ExaPowerIO: 1.327 s (86402 allocations: 809.27 MiB) [ Info: Running JLD2: 61.522 ms (460398 allocations: 174.76 MiB) [ Info: Running PowerModels: 36.598 s (531914384 allocations: 18.62 GiB)
[ Info: Running ExaPowerIO: 1.066 s (80752 allocations: 808.78 MiB) [ Info: Running JLD2: 57.796 ms (460398 allocations: 174.76 MiB) [ Info: Running PowerModels: 34.942 s (531914384 allocations: 18.62 GiB)
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.
@hfytr, here's a second round feedback
| return Expr(:block, body...) | ||
| end | ||
|
|
||
| @inbounds @inline @views function parse_matpower(::Type{T}, ::Type{V}, fname :: String) where {T<:Real, V<:AbstractVector} |
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 function should be broken down into several smaller functions, e.g., _parse_matpower_bus, _parse_matpower_branch, etc.
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 also see a lot of commonality between different types of data. Can we write _parse_matpower function that takes in a tuple of types e.g., (T,T,Int,T,T) and parse the array?
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.
_parse_matpower cant exist because different mpc.() uses different normalizations
i can change @iter_to_ntuple to do something similar
| for row_type in ROW_TYPES | ||
| @test isbitstype(row_type) | ||
| end | ||
| end |
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 should include another Test that test the parser with ExaModelsPower
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.
using out_type=NamedTuple or PowerData?
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.
PowerData, per exanauts/ExaModels.jl#160
The new version will be released soon
|
@hfytr I'll merge this PR for now |
This PR fills out the main functionality of the repo. I'd like to hold off on writing docs till the API is finalized)