-
Notifications
You must be signed in to change notification settings - Fork 2
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
add lazy compute operations #25
base: dev
Are you sure you want to change the base?
Changes from 12 commits
a0a17ea
cda2fd9
2faf11a
5043bc4
a783626
9fbbb8e
1ca659f
294fabc
1f45f1a
cc9dbe0
d8232b8
05d965b
9117ff6
850c170
c1d52af
9d44267
89bca5d
ecbfe08
14d2e00
5bb180d
aaf300a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about having a folder There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should reorganise all the code like that, but in this PR im trying to add new things without completely reorganising the old ones so that changes to the old code diff properly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So for now its just a file with struct definitions and the methods are left in their current place. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
# New type-based interface | ||
# Easier to add parameters to these | ||
abstract type ConnectivityMeasure end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you describe what is the difference between those? I feel like the type system should be as close as possible to the mathematical classification of these measures. I suggest that we could follow the classification of e.g. Graphs.jl or that of networkx: https://networkx.org/documentation/stable/reference/algorithms/index.html i.e., DistanceMeasure, ConnectivityMeasure, CentralityMeasure, etc... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds like a good idea, the GraphMeasures that we discussed would then be CentralityMeasures (connected_habitat is a weighted closeness centrality and the betweennesses are betweenness centralities) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets do a round or two of documenting and reorganising/renaming the type structure when differences get clearer, the objects here can be seen as place holders for better names. |
||
|
||
abstract type FundamentalMeasure <: ConnectivityMeasure end | ||
abstract type RSPDistanceMeasure <: FundamentalMeasure end | ||
|
||
struct LeastCostDistance <: ConnectivityMeasure end | ||
@kwdef struct ExpectedCost{T<:Union{Real,Nothing}} <: RSPDistanceMeasure | ||
θ::T=nothing | ||
approx::Bool=false | ||
end | ||
@kwdef struct FreeEnergyDistance{T<:Union{Real,Nothing}} <: RSPDistanceMeasure | ||
θ::T=nothing | ||
approx::Bool=false | ||
end | ||
@kwdef struct SurvivalProbability{T<:Union{Real,Nothing}} <: FundamentalMeasure | ||
θ::T=nothing | ||
approx::Bool=false | ||
end | ||
@kwdef struct PowerMeanProximity{T<:Union{Real,Nothing}} <: FundamentalMeasure | ||
θ::T=nothing | ||
approx::Bool=false | ||
end | ||
|
||
keywords(cm::ConnectivityMeasure) = _keywords(cm) | ||
|
||
# TODO remove the complexity of the connectivity_function | ||
# These methods are mostly to avoid changing the original interface for now | ||
connectivity_function(::LeastCostDistance) = least_cost_distance | ||
connectivity_function(::ExpectedCost) = expected_cost | ||
connectivity_function(::FreeEnergyDistance) = free_energy_distance | ||
connectivity_function(::SurvivalProbability) = survival_probability | ||
connectivity_function(::PowerMeanProximity) = power_mean_proximity | ||
|
||
# This is not used yet but could be | ||
compute(cm::ConnectivityMeasure, g) = | ||
connectivity_function(m)(g; keywords(cm)...) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
abstract type GraphMeasure end | ||
|
||
keywords(o::GraphMeasure) = _keywords(o) | ||
|
||
abstract type TopologicalMeasure <: GraphMeasure end | ||
abstract type BetweennessMeasure <: GraphMeasure end | ||
abstract type PerturbationMeasure <: GraphMeasure end | ||
abstract type PathDistributionMeasure <: GraphMeasure end | ||
|
||
struct BetweennessQweighted <: BetweennessMeasure end | ||
@kwdef struct BetweennessKweighted{DV} <: BetweennessMeasure | ||
diagvalue::DV=nothing | ||
end | ||
struct EdgeBetweennessQweighted <: BetweennessMeasure end | ||
@kwdef struct EdgeBetweennessKweighted{DV} <: BetweennessMeasure | ||
diagvalue::DV=nothing | ||
end | ||
|
||
@kwdef struct ConnectedHabitat{DV} <: GraphMeasure | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this should be a topological measure |
||
diagvalue::DV=nothing | ||
end | ||
|
||
@kwdef struct Criticality{DV,AV,QT,QS} <: PerturbationMeasure | ||
diagvalue::DV=nothing | ||
avalue::AV=floatmin() | ||
qˢvalue::QS=0.0 | ||
qᵗvalue::QT=0.0 | ||
end | ||
|
||
# These maybe don't quite belong here | ||
@kwdef struct EigMax{F,DV,T} <: TopologicalMeasure | ||
diagvalue::DV=nothing | ||
tol::T=1e-14 | ||
end | ||
struct MeanLeastCostKullbackLeiblerDivergence <: PathDistributionMeasure end | ||
struct MeanKullbackLeiblerDivergence <: PathDistributionMeasure end | ||
|
||
# Kind of a hack for now but makes the input requirements clear | ||
keywords(o::GraphMeasure, dt, p::AbstractProblem) = | ||
(; _keywords(o)..., distance_transformation=dt) | ||
keywords(o::Union{BetweennessQweighted,EdgeBetweennessQweighted,PathDistributionMeasure}, dt, p::AbstractProblem) = | ||
(; _keywords(o)...) | ||
keywords(o::Union{BetweennessKweighted,EigMax}, dt, p::AbstractProblem) = | ||
(; _keywords(o)..., | ||
connectivity_function=connectivity_function(p), | ||
distance_transformation=dt) | ||
keywords(o::ConnectedHabitat, dt, p::AbstractProblem) = | ||
(; _keywords(o)..., | ||
distance_transformation=dt, | ||
connectivity_function=connectivity_function(p), | ||
approx=connectivity_measure(p).approx) | ||
|
||
graph_function(m::BetweennessKweighted) = betweenness_kweighted | ||
graph_function(m::EdgeBetweennessKweighted) = edge_betweenness_kweighted | ||
graph_function(m::BetweennessQweighted) = betweenness_qweighted | ||
graph_function(m::EdgeBetweennessQweighted) = edge_betweenness_qweighted | ||
graph_function(m::ConnectedHabitat) = connected_habitat | ||
graph_function(m::Criticality) = criticality | ||
graph_function(m::EigMax) = eigmax | ||
graph_function(m::MeanLeastCostKullbackLeiblerDivergence) = mean_lc_kl_divergence | ||
graph_function(m::MeanKullbackLeiblerDivergence) = mean_kl_divergence | ||
|
||
compute(m::GraphMeasure, p::AbstractProblem, g::Union{Grid,GridRSP}) = | ||
compute(m, p.distance_transformation, p, g) | ||
compute(m::GraphMeasure, dts::Union{Tuple,NamedTuple}, p::AbstractProblem, g::Union{Grid,GridRSP}) = | ||
map(dts) do dt | ||
compute(m, dt, p, g) | ||
end | ||
compute(m::GraphMeasure, distance_transformation, p::AbstractProblem, g::Union{Grid,GridRSP}) = | ||
graph_function(m)(g; keywords(m, distance_transformation, p)...) |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue that memory use and flops is also related to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes makes sense, memory can just be checked by running one column. Flops may need us to get some values I'm not sure we have access to (but e.g. UMFPACK dose track flops internally). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you are refactoring this, it would make sense to already think of #29. Maybe add a field |
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.
Why not having something like
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've allowed passing solver modes to
\
in rsp, but playing around I found some situations where LinearSolve solvers were very fast on Z but stalled in RSP, so maybe we want different solvers per problem?We also want to factor out some of the solves as the same thing is being done in multiple places if you want multiple kinds of betweenness metrics in the same run.