54 combine conttempnetwork and conttempinstnetwork to single class temporalnetwork

[j-i-l]

[github-actions[bot]]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/flowstab
_cython_sparse_stoch_subst.py
_cython_subst.py
flow_stability.py
parallel_clustering.py
sparse_stoch_mat.py
synth_temp_network.py
src/flowstab/temporal_network
temporal_network.py					54, 87-290, 307-309, 317-353, 414, 495, 515-523, 578, 595-598, 610-616, 622, 628-636, 651-656, 689, 696, 702, 714, 730-733, 744, 754, 760-768, 787-794, 823, 833, 840, 880, 920, 931-938, 957, 965, 1014, 1024, 1041, 1049, 1102, 1112, 1150, 1232, 1239-1240, 1259-1268, 1300, 1344, 1349-1355, 1393, 1401-1404, 1431-1442, 1452-1459, 1512, 1576, 1584, 1597, 1603, 1612-1628, 1641-1648, 1686, 1696, 1707, 1714-1720, 1751-1759, 1767-1772, 1828, 1834, 1862, 1869-1872, 1900, 1919-1936, 1943, 1961-1971, 1982, 1996, 2013, 2023, 2028-2042, 2059, 2074-2082, 2106, 2116-2120, 2134, 2145-2151, 2167-2176, 2183-2198, 2277-2292, 2305-2310, 2422, 2450-2456, 2496, 2513, 2606, 2670, 2680, 2797, 2841
temporal_network_internal.py					95-1951
Project Total

This report was generated by python-coverage-comment-action

[j-i-l]

First we need an example dataset, some minimal test data to construct a temporal network.

[j-i-l]

@alexbovet: What is the rationale behind allowing the setting of node_to_label_dict when initializing a ContTempNetwork?

I understand that this attribute is beneficial as it facilitates the mapping of node IDs to node labels, especially when nodes are relabeled. However, what is the reason for permitting users to define this mapping? If a user intends to remap node labels—such as merging two labels into one—this could easily be accomplished prior to creating the temporal network instance.

The Temporal_Network class could then concentrate solely on tasks related to flow stability.

While it’s tempting to incorporate a wide range of functionalities, doing so can lead to increased complexity and a more complicated user experience, which may quickly outweigh the advantages, particularly for less experienced users.

Also, could we drop the relabel_nodes argument? We should always first create our own node ID's and do all our computations based on them. So we always have a mapping of the provided node labels to our internal IDs. Whenever we export a temp_network we can use the mapping to write out the initially provided labels. This is simple and consistent: the labels that come in are the labels that go out. And if someone wants to access the 'initial IDs' there is our internal mapping that can be used to get them.

[alexbovet]

Yes, this was probably because I needed it at some point. You are proposing to relabel nodes by default and dropping the custom node_to_label_dict? I'm fine with it.

[j-i-l]

Yes, this was probably because I needed it at some point. You are proposing to relabel nodes by default and dropping the custom node_to_label_dict? I'm fine with it.

Great! That would exactly be the idea!

[j-i-l]

@alexbovet: is fix_tau_k used for anything other than handling instantaneous events? From what I see it is only used in case we have instantaneous events. If it is only for instantaneous events we do not need to expose this as parameter that can be set (also falsely) by the user.

[alexbovet]

Just for handling instantaneous events

[j-i-l]

@alexbovet for instantaneous events, is there a reason why the event duration is given by $t_{i+1} - t_{i}$ of unique starting times: https://github.com/alexbovet/flow_stability/blob/85ae8c597b267da65d193a4cfe13ec0412c04ea8/src/flowstab/temporal_network.py#L1925-L1926 and not just $1$, as is the case for the last ending time: https://github.com/alexbovet/flow_stability/blob/85ae8c597b267da65d193a4cfe13ec0412c04ea8/src/flowstab/temporal_network.py#L1927-L1929

Depending on the input data there might be moments with lower interaction density, leading potentially to bigger $t_{i+1} - t_{i}$ an varying "durations" of the instantaneous interactions.

Simple example:

source, target, start
node1, node2, 0
node2, node3, 1
node1, node3, 5

Here the 2nd interaction (node2 - node3) will get a duration of 4while the two other will get a duration of 1.

Should they not all get a duration of 1?

OK, durations is then always set to 1 in the constructor:

https://github.com/alexbovet/flow_stability/blob/85ae8c597b267da65d193a4cfe13ec0412c04ea8/src/flowstab/temporal_network.py#L1942-L1942

Thus, ending_times should not be used for instantaneous temporal networks.

[j-i-l]

@alexbovet for the ContTempInstNetwork class, the laplaican computation is done differently. However, temporal networks with instantaneous events are treated like regular temporal networks with events durations = 1, right?!

Why is the laplacian computation different? And why do we not get the same laplacians for a ContTempNetwork with event durations all set to 1 and ContTempInstNetwork with identical source, target, and starts? (here is the missmatch).

[j-i-l]

@alexbovet in the instantaneous events case we set the event durations to 1. However, when we compute the inter event transitions matrices, $\tau_k$ is given by the inter-event time, which is not necessarily 1 (e.g. one event starts at $t=1$ and the next at $t=1.1$).

It is a bit unclear to me, why it is ok in this case to fix $\tau_k=1$ and not just to $\tau_k=t_{k+1} - t_{k}$ as is the case for non-instantaneous events.

Would it not be an option to treat the instantaneous case just like the non-inst. one?

[alexbovet]

@alexbovet for the ContTempInstNetwork class, the laplaican computation is done differently. However, temporal networks with instantaneous events are treated like regular temporal networks with events durations = 1, right?!

Why is the laplacian computation different? And why do we not get the same laplacians for a ContTempNetwork with event durations all set to 1 and ContTempInstNetwork with identical source, target, and starts? (here is the missmatch).

When you have events with durations, the end of an event creates a new inter event time and a new laplacian. So they instantaneous Laplacians will be different.

I think the implementation is not great. Actually, it should not be possible to enter ending_times for instantaneous tempnet. and durations should not be equal to 1, but to None or just non-existant.

@alexbovet in the instantaneous events case we set the event durations to 1. However, when we compute the inter event transitions matrices, τ k is given by the inter-event time, which is not necessarily 1 (e.g. one event starts at t = 1 and the next at t = 1.1 ).

It is a bit unclear to me, why it is ok in this case to fix τ k = 1 and not just to τ k = t k + 1 − t k as is the case for non-instantaneous events.

Would it not be an option to treat the instantaneous case just like the non-inst. one?

With instantaneous tempnet, fix_tau_k is set to True (hence tau_k=1 for all events) and the inter event times do not matter. This is because with instantaneous events, the diffusion dynamics only depends on lambda.

[j-i-l]

@alexbovet for the ContTempInstNetwork class, the laplaican computation is done differently. However, temporal networks with instantaneous events are treated like regular temporal networks with events durations = 1, right?! Why is the laplacian computation different? And why do we not get the same laplacians for a ContTempNetwork with event durations all set to 1 and ContTempInstNetwork with identical source, target, and starts? (here is the missmatch).

When you have events with durations, the end of an event creates a new inter event time and a new laplacian. So they instantaneous Laplacians will be different.

I think the implementation is not great. Actually, it should not be possible to enter ending_times for instantaneous tempnet. and durations should not be equal to 1, but to None or just non-existant.

@alexbovet in the instantaneous events case we set the event durations to 1. However, when we compute the inter event transitions matrices, τ k is given by the inter-event time, which is not necessarily 1 (e.g. one event starts at t = 1 and the next at t = 1.1 ). It is a bit unclear to me, why it is ok in this case to fix τ k = 1 and not just to τ k = t k + 1 − t k as is the case for non-instantaneous events. Would it not be an option to treat the instantaneous case just like the non-inst. one?

With instantaneous tempnet, fix_tau_k is set to True (hence tau_k=1 for all events) and the inter event times do not matter. This is because with instantaneous events, the diffusion dynamics only depends on lambda.

Ah, now it becomes clearer. The approach is not to actually set an event duration of 1, but to simple consider starting times for the inter event times in the case of instantaneous events. In this case the computation of the inter-event transition matrices uses $\tau_k=1$ not because of the duration but because 1 is the neutral element in multiplication. It is a way to say there is no $\tau_k$ in the context of instantaneous events.

We could indeed implement this somewhat clearer, I think. Should I have a go at it?