Created attachment 990 [details]
Proposed class

this is really for nicola to review.

I like very much the idea! 

As for the implementation, I have a few comments:


1) can you please also modify the implementation of the pre-existing code as follows?

void
MatrixPropagationLossModel::SetLoss (Ptr<Node> a, Ptr<Node> b, double loss, bool symmetric)
{
  Ptr<MobilityModel> ma = a->GetObject<MobilityModel> ();
  Ptr<MobilityModel> mb = b->GetObject<MobilityModel> ();
  SetLoss (ma, mb, loss, symmetric);
}

So that we reuse code instead of duplicating it;


2) some proposed renaming:

PropagationLossCachedEvaluator 
--> PropagationLossCache

MatrixPropagationLossModel::ContainsPair ()
--> MatrixPropagationLossModel::IsLossSet ()
(for analogy with MatrixPropagationLossModel::SetLoss ())


3) it would be nice if you could provide an example program using this propagation loss model.


Once the above issues are properly addressed, I think it should be ok to push this code.


(In reply to comment #2)
> this is really for nicola to review.

Ok I did it, but you're the maintainer of src/common, so it's your call :-)

(In reply to comment #3)
> > this is really for nicola to review.
> 
> Ok I did it, but you're the maintainer of src/common, so it's your call :-)

I don't consider myself maintainer of the wireless bits in there. I pre-approve whatever you think is right.

Just wondering - is it possible to do the AddModel operation via attributes? The current implementation doesn't support YansWifiChannelHelper (you need to instantiate the cache first before you can wrap other models onto it).

I have no idea how to go about implementing it, but it would be good if one can do something like:
// YansWifiChannelHelper wifiChannel;
wifiChannel.AddPropagationLoss ("ns3::PropagationLossCache", "ns3::SomePorpagationLossModel", "Attribute0", value0, ..., "AttributeN", valueN,
"ns3::AnotherLossModel", "Attribute0", value0,...);

Created attachment 991 [details]
updated patch

Changes made as suggested.

I've also added a helper class for it.

Sample code snippet in the doxygen code comments.

(In reply to comment #6)
> Created an attachment (id=991) [details]
> updated patch
> 
> Changes made as suggested.
> 
> I've also added a helper class for it.
> 
> Sample code snippet in the doxygen code comments.

1) I would suggest to change the attribute documentation from:

"Whether propagation loss is symmetric",
to 
"Whether propagation cache is symmetric",

2) In the matrix model itself, suggest to change this (existing) doxygen:
-   * \param symmetric   If true (default), both a->b and b->a paths will be affected
+   * \param symmetric   If true (default), both a->b and b->a paths will be set

3) I have a broader comment about the below new documentation comment, since I just left a similar note for bug 1008:

"The wrapped models MUST be independent of the input signal strength."

It seems like we will have the capability to "chain" and to "cache" these loss models, but we are not enforcing this in the code, but rather relying on users to read the documentation and not make a mistake.  We have loss models that do not satisfy these properties yet they can be cached or chained.  

Should we introduce capability queries that are tested in SetNext() and AddModel() before allowing these models to be added into these structures?

Created attachment 1001 [details]
updated docs

Documentation updated as suggested.

Would be good if they can be checked programmatically (probably on a separate ticket). Maybe an abstract class "CommutativePropagationLossModel" for compile-time enforcement, and/or a bool IsCommutative() function for runtime checks?

I feel that if we add this method to the base class, maybe we should add the caching code to the base class too.

I don't think caching is useful in the general case - if any node moves, then the results cannot be cached (unless we also keep a list of what nodes can move, but I'm not sure whether this is worth the extra effort).

(In reply to comment #10)
> I don't think caching is useful in the general case - if any node moves, then
> the results cannot be cached (unless we also keep a list of what nodes can
> move, but I'm not sure whether this is worth the extra effort).

I tend to agree with Mathieu's comment.  What about disabling caching by default (e.g an attribute), but allow user to enable it if they feel that their simulation might benefit.  Then, implement some type of a LRU cache whose size is also configurable.  

If we make the cache key a function of all of the input parameters of CalcRxPower() then we can avoid having to distinguish between models that are commutative and those that are not.

(In reply to comment #11)
> I tend to agree with Mathieu's comment.  What about disabling caching by
> default (e.g an attribute), but allow user to enable it if they feel that their
> simulation might benefit.  Then, implement some type of a LRU cache whose size
> is also configurable.  

Now I understand what you meant - we add caching capability to the base class PropagationModel (ie. each separate instance of PropagationModel), disabled by default. Compared to the current proposal (which caches the results of the wrapped chain), it takes more lookups to get the final answer (one per Model), but is more general. I'll play around with it and see what I can come up with (will probably need help with the LRU bits later).

> If we make the cache key a function of all of the input parameters of
> CalcRxPower() then we can avoid having to distinguish between models that are
> commutative and those that are not.

In this case, we no longer need the models to be commutative. We do however need to separate stochastic models from deterministic models - we should not allow users to accidentally enable cache on stochastic models. We should also have ways for the Model authors to key certain inputs from the cache key (eg. commutative models don't need Rx power as part of the key).

Maybe we can:
* In PropagationModel, add:
** abstract function DoCacheLookup, and DoCacheResult
** boolean attribute CacheEnabled
** on Propagationmodel::CalcRxPower, if CacheEnabled, call DoCacheLookup, then if cache miss, call DoCalcRxPower and DoCacheResult. If !CacheEnabled, call DoCalcRxPower.

* Add new abstract classes StochasticPropagationModel and DeterministicPropagationModel (or maybe Cacheable/NonCacheablePropagationModel). Add caching code in DeterministicPropagationModel, and make StochasticPropagationModel::DoCacheLookup call DoCalcRxPower.

* children of Deterministic/Stochastic need only implement the DoCalc function as per current arrangements.

(In reply to comment #12)
> Maybe we can:
> * In PropagationModel, add:
> ** abstract function DoCacheLookup, and DoCacheResult
> ** boolean attribute CacheEnabled
> ** on Propagationmodel::CalcRxPower, if CacheEnabled, call DoCacheLookup, then
> if cache miss, call DoCalcRxPower and DoCacheResult. If !CacheEnabled, call
> DoCalcRxPower.
> 
> * Add new abstract classes StochasticPropagationModel and
> DeterministicPropagationModel (or maybe
> Cacheable/NonCacheablePropagationModel). Add caching code in
> DeterministicPropagationModel, and make
> StochasticPropagationModel::DoCacheLookup call DoCalcRxPower.
> 
> * children of Deterministic/Stochastic need only implement the DoCalc function
> as per current arrangements.

This would work I think. But, I have to ask: is it really such a big performance gain that it's worth doing all this ?

(In reply to comment #13)
> This would work I think. But, I have to ask: is it really such a big
> performance gain that it's worth doing all this ?

I'm implementing a shadowing model where each obstruction is modelled as a rectangle with a fixed attenuation. When DoCalcRxPower is called, it iterates through all obstructions and calculates whether the straight line between the source and receiver intersects any sides.

Therefore each transmission is of order:
O(n*m), where n = #nodes, m = #obstructions (intersection calculation is also expensive due to all those square and sqrt operations as well). The current version of the cache reduces that down to O(log2(n(n-1))) ~ O(log2(n)). If I change map to a hash_map, I can hopefully drop that down to O(1)

NS-3 already has something similar in src/propagation/model/propagation-cache.h

Not that I think this patch should be rejected, it solves different problem, allows asymmetric propagation etc., just to avoid code duplication in case it is merged eventually.

The proposed patch is 6 years old now, I am closing it for lack of activity. Feel free to reopen if you think that the functionality is useful and are willing to dedicate effort to update the patch to the current code base and get it integrated in ns-3-dev.