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

Feature Request: Defined access to GraphData #58

Closed
hexaeder opened this issue Nov 9, 2020 · 2 comments · Fixed by #59
Closed

Feature Request: Defined access to GraphData #58

hexaeder opened this issue Nov 9, 2020 · 2 comments · Fixed by #59

Comments

@hexaeder
Copy link
Member

hexaeder commented Nov 9, 2020

As mentioned in #54 (comment) I'd like to propose a clearly defined set of accessors to get the underlying data of a graph. I think this is a major need for #49. Since it defines a new set of functions for the end users this interface should be designed very carefully though.

From my understanding the main accessors are

# get the 'view' for the i-th vertex
function get_vertex(gd::GraphData, v_idx) :: AbstractVector{elE}

# get the 'view' for the i-th edge
function get_edge(gd::GraphData, e_idx) :: AbstractVector{elE}

# get the 'view' to the src vertex of the i-th edge
function get_src_vertex(gd::GraphData, e_idx) :: AbstractVector{elV}

# get the 'view' to the dst vertex of the i-th edge
function get_dst_vertex(gd::GraphData, e_idx) :: AbstractVector{elV}

# get all the 'views' to the outgoing edges of the i-th vertex
function get_out_edges(gd::GraphData, v_idx) :: AbstractVector{AbstractVector{elE}}

# get all the 'views' to the incomming edges of the i-th vertex
function get_in_edges(gd::GraphData, v_idx) :: AbstractVector{AbstractVector{elE}}

These are the ones used in the core loop. Replacing this would already help decoupling the NetworkStructures with the rest of the package, for example one could change to Julias build-in views without changing another file. It also simplifies testing of different implementations.

These accessors could be overloaded, for example to get multiple edges at once. One could also think of other useful accesors, for example to get access based on the symbols. Right now, this is done via

function view_v(gd::GraphData, gs::GraphStruct, sym="")

which uses the GraphStruct. I think methods like this should be available for the user without even knowing about the existence of GraphStruct. Once the GraphDataBuffer is in place one should consider why GraphStruct and GraphData are even separate types since they strongly depend on each other.

further thoughts:

  • this is somewhat related to redundant linear indexing #52 because it would solve the uncertainty about the right way to access the data
  • strongly related to GetGD, GetGS and ND_Solution #35, one has to think about how to keep the static values in the GraphData up to date with changes to the non-static part (i.e. don't recalculate static edges if the vertex data hasn't changed).
  • right now a lot of the stuff in NetworkStructures serves the purpose of caching (i.e. all possible queries like the outgoing edges for every vertex are precomputed during setup). Once one allows access to graph data based on symbols in callbacks it is not possible to compute all of the 'answers' during setup. Maybe one could look into a more automatic approach of Memoization based on the arguments of each accessor function.
@hexaeder
Copy link
Member Author

Inside ND.jl there was a last function which depended on a specific field of GraphData.

function (sef::StaticEdgeFunction)(x, p, t)
    gd = sef.nd_ODE_Static(x, p, t, GetGD)

    (gd.e_s_v, gd.e_d_v)
end

I got rid of it by introducing allocations. Would be good to know whether this function is used by PD.jl in a way which can't be done via the new accessors.

@hexaeder
Copy link
Member Author

Okay it is used at least in

at least these are the ones which can be found easily. I think it would be now problem to change the calls there. They don't need the full gd.e_s_v which is an Array of Arrays. Instead they are only interested in gd.e_s_v[i] for a specific vertex i which can be achieved with get_in_edges and get_out_edges.
So since 0.5 will be breaking anyway it might be okay to just get rid of the function?

@lindnemi lindnemi linked a pull request Nov 11, 2020 that will close this issue
@lindnemi lindnemi reopened this Feb 19, 2021
@lindnemi lindnemi closed this as completed Apr 1, 2021
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 a pull request may close this issue.

2 participants