Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

decouple GraphStructure from other modules through accessors #59

Merged
merged 15 commits into from
Nov 16, 2020
Merged

decouple GraphStructure from other modules through accessors #59

merged 15 commits into from
Nov 16, 2020

Conversation

hexaeder
Copy link
Member

@hexaeder hexaeder commented Nov 9, 2020

this is an resolves #58. Not meant for merging right now.

The goal of this is to have a clear interface how to interact with the
GraphData which doesn't depend on the concrete implementation.
@lindnemi
Copy link
Collaborator

lindnemi commented Nov 9, 2020

Looks good!

@hexaeder hexaeder changed the base branch from master to dev-0.5 November 11, 2020 09:49
This is a bad solution since it is allocating. But is it actually needed by
PD.jl?
@codecov-io
Copy link

codecov-io commented Nov 11, 2020

Codecov Report

❗ No coverage uploaded for pull request base (dev-0.5@95d7a79). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             dev-0.5      #59   +/-   ##
==========================================
  Coverage           ?   67.46%           
==========================================
  Files              ?        7           
  Lines              ?      375           
  Branches           ?        0           
==========================================
  Hits               ?      253           
  Misses             ?      122           
  Partials           ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95d7a79...2147fe2. Read the comment docs.

@lindnemi
Copy link
Collaborator

Yes, PD uses StaticEdgeFunction, so we have to either rewrite that part of PD or get rid of the allocations again: https://github.com/JuliaEnergy/PowerDynamics.jl/search?q=StaticEdgeFunction.

Btw, it looks like it was allocating before, since it returns a tuple. Did allocations increase?

@hexaeder
Copy link
Member Author

Tuples are of known length and size and can be stack allocated. Before it was returning a tuple containing of two pointers to heap allocatet Arrays e_s_v and e_s_d which have been allocated before therefore non allocating function call.

Now it is composing a tuple (non allocating) of to pointers but the pointers refer to arrays which will be created in the function call through these iterators (allocating). But the PD packages doesn't seem to be interested in the full arrays anyway (see my comment)

@hexaeder hexaeder marked this pull request as ready for review November 11, 2020 12:59
@lindnemi lindnemi linked an issue Nov 11, 2020 that may be closed by this pull request
@hexaeder hexaeder changed the title define interface to access underlying graph data decouple GraphStructure from other modules through accessors Nov 11, 2020
@hexaeder
Copy link
Member Author

hexaeder commented Nov 11, 2020

Should not be merged yet. I'll try to decouple the GraphStructure from the rest of the package. As discussed earlier it would be nice to change the module (for example switching to the GraphDataBuffer-design) without having to touch a lot of code outside of NetworkStructures.jl.

@lindnemi
Copy link
Collaborator

Nice to see tests for the modules and for allocations. Did you do the benchmarks yet?

Also I think GetGD and GetGS are abusing multiple dispatch and could be functions instead of types. For what reason were they chosen to be types @FHell ?

@hexaeder
Copy link
Member Author

Yes, no perfomance hit compared to master (i think everything boils down to the same machine code anyway due to inlining).

Also I think GetGD and GetGS are abusing multiple dispatch and could be functions instead of types. For what reason were they chosen to be types @FHell ?

I guess because of the type of nd

nd = network_dynamics(nd_diffusion_vertex, nd_diffusion_edge, g)
typeof(nd) ## isa ODEFunction
typeof(nd.f) ## isa nd_ODE_Static

The ODEFunction forwards all function calls to its .f field. In order to put in in a function one would have to define a new method on the ODEFunction object.

function getGD(nd::ODEFunction{true, T}) where T<:nd_ODE_Static
    nd.f.graph_data
end

(in case you are wondering, the first type parameter true for ODEFunctions is whether it is in place.)

I think this not high priority but certainly something to think about (#35).

@FHell FHell merged commit 36691c3 into JuliaDynamics:dev-0.5 Nov 16, 2020
@hexaeder hexaeder deleted the gd_access branch November 16, 2020 14:58
lindnemi pushed a commit that referenced this pull request Mar 19, 2021
decouple GraphStructure from other modules through accessors
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Defined access to GraphData
4 participants