Skip to content
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

Refactor network data #6

Merged
merged 10 commits into from May 26, 2019
Merged

Refactor network data #6

merged 10 commits into from May 26, 2019

Conversation

FHell
Copy link
Member

@FHell FHell commented May 22, 2019

This is a big refactor in preparation for work on adding more Type awareness to the system. Some of the old API that I believe we never used is deprecated in this version. The internal structure now relies on the Vertex|EdgeFunction types directly, and the network dynamics retains the full information in its internal fields.

All the logic for calculating the indices, etc, that was duplicated all over the place now has a clear place, the indices and internal variables were combined into a struct that lives in NetworkStructures

The tests work, but they are rather limited. Could you please check if your branch of PowerDynamics works against this? Also happy to hear about what you think about the design as such.

@FHell FHell requested a review from janlisse May 22, 2019 22:57
@janlisse
Copy link
Collaborator

janlisse commented May 23, 2019

When i run the branch against our PowerDynamics example i get this error:

ERROR: LoadError: UndefVarError: sparse not defined
Stacktrace:
[1] construct_mass_matrix(::Array{Union{Nothing, Array{Int64,1}},1}, ::Int64, ::GraphStructure) at /Users/jan/.julia/dev/NetworkDynamics/src/NetworkStructures.jl:77
 [2] network_dynamics(::Array{ODEVertex,1}, ::Array{StaticEdge,1}, ::LightGraphs.SimpleGraphs.SimpleGraph{Int64}) at /Users/jan/.julia/dev/NetworkDynamics/src/NetworkDynamics.jl:48
 [3] read_network_from_csv(::String, ::String) at /Users/jan/projects/julia/PowerDynBase.jl/src/parsers/csv_parser.jl:11
 [4] top-level scope at none:0
in expression starting at /Users/jan/projects/julia/PowerDynBase.jl/ieee14-minimal-example.jl:10

@janlisse
Copy link
Collaborator

janlisse commented May 24, 2019

Ok, i fixed the above error myself by adding a "using SparseArrays" to NetworkStructures.jl
Now i get another error:

ERROR: LoadError: MethodError: Cannot `convert` an object of type Nothing to an object of type Float64
Closest candidates are:
  convert(::Type{T<:Number}, ::T<:Number) where T<:Number at number.jl:6
  convert(::Type{T<:Number}, ::Number) where T<:Number at number.jl:7
  convert(::Type{T<:Number}, ::Base.TwicePrecision) where T<:Number at twiceprecision.jl:250
  ...
Stacktrace:
 [1] fill!(::SubArray{Float64,2,SparseMatrixCSC{Float64,Int64},Tuple{Array{Int32,1},Array{Int32,1}},false}, ::Nothing) at ./multidimensional.jl:837
 [2] copyto! at ./broadcast.jl:805 [inlined]
 [3] materialize!(::SubArray{Float64,2,SparseMatrixCSC{Float64,Int64},Tuple{Array{Int32,1},Array{Int32,1}},false}, ::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Nothing,typeof(identity),Tuple{Base.RefValue{Nothing}}}) at ./broadcast.jl:756
 [4] construct_mass_matrix(::Array{Union{Nothing, Array{Int64,1}},1}, ::Int64, ::GraphStructure) at /Users/jan/.julia/dev/NetworkDynamics/src/NetworkStructures.jl:81

Before the refactoring the code handled the case of the mass_matrix being Nothing. This
special treatment has been removed and that is the cause for the error.

Copy link
Collaborator

@janlisse janlisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the refactoring makes much sense and looks good to me in general!
We need to fix the above error though.
I also was wondering why you changed:
dim_nd = sum(dim_e) + sum(dim_v)
to:
dim_nd = sum(dim_v)
in
function network_dynamics(vertices!::Array{ODEVertex}, edges!::Array{ODEEdge}, graph)

Shouldn't the total dimension include the dynamic edge variables?

And one additional note: The DDE parts still use the old code path not the refactored version.

@FHell
Copy link
Member Author

FHell commented May 24, 2019

First one is a breaking API change. Before we had the incorrect default of mass_matrix=nothing, the correct default is mass_matrix=I, if you were passing mass_matrix=nothing manually this needs to be adjusted.

Second one is a copy paste bug. I will add a test for this.

@janlisse
Copy link
Collaborator

After adding a few more fixes our PowerDynamic examples are running again using this branch.
From my side it is now good to go for a merge. @FHell Shall we merge then?

@FHell FHell merged commit f8f952a into master May 26, 2019
@FHell FHell deleted the refactor-network-data branch May 26, 2019 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants