Issue 5728062: ns-3 buildings module from the LENA project
|
Created:
12 years, 1 month ago by Nicola Baldo Modified:
11 years, 12 months ago CC:
ns-3-reviews_googlegroups.com, Marco Miozzo Visibility:
Public. |
DescriptionPatch Set 1 #
Total comments: 11
Patch Set 2 : GridBuildingAllocator now reusing GridPositionAllocator #Patch Set 3 : decouple propagation models from buildings module #Patch Set 4 : modifications to src/propagation #
Total comments: 54
Patch Set 5 : summary of proposed changes to propagation+buildings #Patch Set 6 : added separate test suites for new propagation models #
Total comments: 1
MessagesTotal messages: 12
I have two main comments: 1) I feel that there is _some_ duplicate code that could be avoided. i.e., the BuildingContainer, BuildingList, BuildingAllocator. A combination of templating NodeList, NodeContainer & co + maybe subclassing GridAllocator and modifying the base class a bit would help. 2) I feel a bit uneasy about the use of the static BuildingList. Could this not be a single object that the mobility model & propagation loss models have a pointer to ? http://codereview.appspot.com/5728062/diff/1/src/buildings/examples/buildings... File src/buildings/examples/buildings-pathloss-profiler.cc (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/examples/buildings... src/buildings/examples/buildings-pathloss-profiler.cc:70: static Ptr<Building> building1 = Create<Building> (0.0, 10.0, 0.0, 10.0, 0.0, 20.0 /*, 1, 1, 1*/); static ? http://codereview.appspot.com/5728062/diff/1/src/buildings/helper/building-al... File src/buildings/helper/building-allocator.h (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/helper/building-al... src/buildings/helper/building-allocator.h:40: class GridBuildingAllocator : public Object There is so much duplicated code with the GridAllocator & co that maybe it would make sense to try to look into sharing code ? http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc File src/buildings/model/building.cc (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:58: TypeId::ATTR_GET, if you provide a getter only as below, you do not need to specify this flag. http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:85: Building::Building (double xMin, why not just remove the declaration of this method from the header ? http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:96: << "Ptr<Building> b = CreateObject<building> (" typo: building -> Building http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.h File src/buildings/model/building.h (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.h#n... src/buildings/model/building.h:225: //std::ostream &operator << (std::ostream &os, const Box &box); kill dead code
Sign in to reply to this message.
Hi Mathieu, thanks for your time and effort. Please find my comments below. > 1) I feel that there is _some_ duplicate code that could be avoided. i.e., the > BuildingContainer, BuildingList, BuildingAllocator. A combination of templating > NodeList, NodeContainer & co + maybe subclassing GridAllocator and modifying the > base class a bit would help. Indeed there is a lot of duplication. Let's discuss case by case: Container: this is not a new duplication introduced by the buildings module, in the current ns-3-dev there are already the following duplications: src/energy/helper/energy-source-container.h src/energy/model/device-energy-model-container.h src/network/helper/node-container.h src/network/helper/application-container.h src/network/helper/net-device-container.h src/internet/helper/ipv6-interface-container.h src/internet/helper/ipv4-interface-container.h The latter two are somewhat more different, but all the others seem almost identical. I agree the Container class could probably be templated. However it's not a trivial effort, and I think this should not be dealt with as part of merge of the buildings model, but rather as a separate issue. Note that dealing with this takes a lot of efforts because of the number of existing duplicates (often with customizations) to be harmonized. List: also this duplication was not "invented" for the buildings module, we have already an occurrence of duplication in ns-3-dev: src/network/model/node-list.h src/network/model/channel-list.h This could be templated as well. It's easier than for Container, because only two such classes currently exists. However we would need to introduce an API change because some method names currently include the class name (e.g., ChannelList::GetChannel(), ChannelList::GetNChannels()). Again, I think it's more appropriate to handle this as a separate issue rather than doing it as part of the revision of the buildings module. (maybe we should post all the above to ns-developers for further discussion) GridAllocator: in this case we don't have (to my knowledge) other cases of similar duplication in ns-3-dev. However, there are some key API differences between GridPositionAllocator and GridBuildingAllocator: 1) PositionAllocator is meant to be used within a MobilityHelper with the GetNext () method, whereas GridBuildingAllocator is called directly by the user from main(). 2) the meaning of the attributes DeltaX and DeltaY change, because PositionAllocator allocates point-type objects, whereas GridBuildingAllocator allocates box-type object with a size that need to be accounted for. In fact, GridBuildingAllocator has additional attributes LengthX and LengthY to handle this, and the actual allocation code is slightly different. because of these API differences, I see no clear benefit in trying to abstract or templatize some common functionality between GridPositionallocator and GridBuildingAllocator. > > 2) I feel a bit uneasy about the use of the static BuildingList. Could this not > be a single object that the mobility model & propagation loss models have a > pointer to ? The design of the buildings mobility and propagation loss models is that BuildingsMobilityModel (when indoor) has a Ptr<Building>, which is then checked by BuildingsPropagationLossModel for its calculation. So BuildingList is not needed for this purpose. The main use case for BuildingList is to implement BuildingsHelper::MakeConsistent (). In a previous version of the code, the user needed to set explicitly for every MobilityModel if it was indoor, and if so to set the Ptr<Building> of the corresponding building and the X and Y room numbers. This proved to be very cumbersome for the user, as well as bug-prone. To fix this, we implemented BuildingsHelper::MakeMobilityModelConsistent (), which goes through the list of all nodes and of all buildings, and automatically sets the parameters of each BuildingsMobilityModel. Thanks to this, the user just needs to provide the (x,y,z) coordinates of every node. One thing we could do is to avoid the static list, and make the above method work with a BuildingContainer instead. But I don't see what's wrong with the static building list in principle. Furthermore, it's nice to have the buildings list so that buildings can be managed with the config store. http://codereview.appspot.com/5728062/diff/1/src/buildings/examples/buildings... File src/buildings/examples/buildings-pathloss-profiler.cc (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/examples/buildings... src/buildings/examples/buildings-pathloss-profiler.cc:70: static Ptr<Building> building1 = Create<Building> (0.0, 10.0, 0.0, 10.0, 0.0, 20.0 /*, 1, 1, 1*/); On 2012/03/06 11:33:30, Mathieu Lacage wrote: > static ? just a cut&paste issue, this code was copied from test/buildings-pathloss-test.cc where "static" was needed for a special reason. It's not need here so I'll remove it. http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc File src/buildings/model/building.cc (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:58: TypeId::ATTR_GET, On 2012/03/06 11:33:30, Mathieu Lacage wrote: > if you provide a getter only as below, you do not need to specify this flag. Ok thanks will do. http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:85: Building::Building (double xMin, On 2012/03/06 11:33:30, Mathieu Lacage wrote: > why not just remove the declaration of this method from the header ? That's a very recent API change we introduced, before using this constructor was the only way. Since there are some people already using our code, we preferred to give them some clue why their existing sim programs suddenly start to fail, and how to fix them. http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.cc#... src/buildings/model/building.cc:96: << "Ptr<Building> b = CreateObject<building> (" On 2012/03/06 11:33:30, Mathieu Lacage wrote: > typo: building -> Building ok http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.h File src/buildings/model/building.h (right): http://codereview.appspot.com/5728062/diff/1/src/buildings/model/building.h#n... src/buildings/model/building.h:225: //std::ostream &operator << (std::ostream &os, const Box &box); On 2012/03/06 11:33:30, Mathieu Lacage wrote: > kill dead code ok sure
Sign in to reply to this message.
Hi all, I've uploaded two patches that, I believe, satisfactorily address Mathieu's review of the buildings module (including also the issues that we discussed at wns3). Here are the details: Patch Set 2 : GridBuildingAllocator now reusing GridPositionAllocator previosuly, GridBuildingAllocator included some code copied from GridPositionAllocator. Now, this is avoided by having GridBuildingAllocator using under the hood two instances of GridPositionAllocator, one for the lower left corner and one for the upper right corner of each building as seen on the X-Y plane. Patch Set 3 : decouple propagation models from buildings module this patch decouples all the propagation models previously embedded in BuildingsPropagationLossModel, so that they can be used stand-alone. Most of them have been moved into src/propagation, with the exception of ItuR1238 which depends on the BuildingsMobilityModel, and which is therefore left in src/buildings Issues raised by Mathieu that have not been adressed: a) duplication of *Container: as I said before, I think that there are already so many instances of this duplication in ns-3-dev (see my previous post) that I think it is not appropriate to consider this a blocking issue for the merge of the LENA code. It should be dealt with as an ns-3 bug. b) BuildingList: this is needed to 1) allow a consistency check upon simulation configuration, and 2) allow the list of buildings to appear in the attribute system so that, for example, the buildings and their attributes can be browsed using the GtkConfigStore. Hence, I think the BuildingList should be not removed. Feedback welcome! Regards, Nicola
Sign in to reply to this message.
I've just uploaded patch set 4 in order to show only the changes that would apply to src/propagation. The code modifications are a subset of those in patch set 3; additionally, the .rst modification is updated. Pavel, you're the maintainer of the propagation module, can you give your +1 to this patch set 4?
Sign in to reply to this message.
On 2012/04/27 11:10:01, Nicola Baldo wrote: > additionally, the .rst modification is updated. sorry, I meant "the .rst documentation is updated"
Sign in to reply to this message.
Hi guys, I've left a number of fast comments in some files, I'm going to return to this later. Currently the main objection to the merging the code -- I see no unit tests. Best regards, Pavel http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:41: People tend to have explicit getters and setters as well, I am personally neutral about this. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:45: "The Frequency (default is 2.106 GHz).", Strange default! Nobody will actually know this. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:121: NS_FATAL_ERROR (this << " Street Orientation must be in [0,90]"); This is better done by NS_ASSERT since you already check [0, 90] range in the attribute setter. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:130: double pi = 3.141592653589793; Well, if you don't believe to M_PI from <cmath> use const double at least. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:223: A LOT of unit tests should be here or I will never believe that expressions above are correct. Will try to find tests in separate files. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h:30: Doxygen class comment is wanted. Don't forget to add \ingroup propagation http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h:40: * Set the operating frequency The same comment about labmda as for Hata-Okumura. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... File src/propagation/model/okumura-hata-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:38: OkumuraHataPropagationLossModel::GetTypeId (void) Explicit setter/getter methods for the attributes can be useful. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:44: .AddAttribute ("Frequency", This is not consistent with Friis and COST231 Hata propagation loss model, where carrier frequency is parametrized as Lambda in meters. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:77: // standard Okumura Hata (from wikipedia) I'd personally prefer more "scientific" sources, but ok -- just add URL and copy this comment to the header (class comment) http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:100: // NS_LOG_INFO (this << " logf " << 26.16 * log_f << " loga " << log_aHeight << " X " << (((44.9 - (6.55 * log10(hb)) ))*log10 (a->GetDistanceFrom (b))) << " logb " << log_bHeight); Commented source code is bad http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:112: else if (m_frequency <= 2.170e9) // max 3GPP freq EUTRA band #1 and I am surprised why Hata-Okumura are related to the 3GPP frequency bands? http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:138: // "Path Loss Models for Suburban Scenario at 2.3GHz, 2.6GHz and 3.5GHz" Copy me to the header http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:143: return loss; I see no branch for large frequencies, it seems loss = 0 for freq > 2.69GHz! Will return to this later. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:154: Where are the tests? It is generally convenient to have unit tests right below the tested code. Will try to find tests later. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... File src/propagation/model/okumura-hata-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.h:30: Doxygen class comment wanted http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.h:40: * Empty lines? http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... File src/propagation/model/propagation-environment.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... src/propagation/model/propagation-environment.h:31: * The type of propagation environment \ingroup propagation http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... src/propagation/model/propagation-environment.h:40: /** \ingroup propagation
Sign in to reply to this message.
The rest of the review. RST docs are great, but no tests found yet. http://codereview.appspot.com/5728062/diff/13001/src/propagation/doc/propagat... File src/propagation/doc/propagation.rst (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/doc/propagat... src/propagation/doc/propagation.rst:60: OkumuraHataPropagationLossModel Very good, just copy some basic things and references to the header comments. Some people might prefer doxygen exploring the propagation models. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-los-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:45: DoubleValue (2160e6), The same as above on default value http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:55: NS_LOG_FUNCTION (this); alignment http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:60: double Lbp = fabs (20 * log10 ((m_lambda * m_lambda) / (8 * M_PI * a->GetPosition ().z * b->GetPosition ().z))); oops, M_PI here and double pi = 3.1415... in another file. Be consistent. Using M_PI don't forget to define _USE_MATH_DEFINES http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:82: { I'd add NS_ASSERT (freq > 0) http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:94: No tests :( http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-los-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.h:29: Doxygen comment wanted. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.h:38: /** The same as above on frequency vs. lambda
Sign in to reply to this message.
Hi Pavel, thanks for your review. I'll address the detailed comments next Monday. In the meanwhile, I'd just like to comment that the test coverage is provided by the test suite in the buildings module, in particular by src/buildings/test/buildings-pathloss-test.cc which was included in patch set 1: http://codereview.appspot.com/5728062/patch/1/34 these tests verify the path loss values returned by HybridBuildingsPropagationLossModel. Since HybridBuildingsPropagationLossModel uses under the hood the three models OkumuraHata, ItuR1411Los and ItuR1411NlosOverRooftop, these latter models are covered. In fact there are separate test cases for each.
Sign in to reply to this message.
Please find below my answers to Pavel's comments based on how I addressed them in revising the code. I'll upload a new patch shortly. To summarize, all issues have been addressed, with the exception of: - the Lambda vs Frequency attribute issue, which deserves more discussion (it's a user interface issue) but which I think would be better postponed to after the merge; - the _USE_MATH_DEFINES issue which actually affects all ns-3, not only this code being reviewed; hence, should be dealt with separately. http://codereview.appspot.com/5728062/diff/13001/src/propagation/doc/propagat... File src/propagation/doc/propagation.rst (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/doc/propagat... src/propagation/doc/propagation.rst:60: OkumuraHataPropagationLossModel On 2012/04/28 14:15:03, Pavel Boyko wrote: > Very good, just copy some basic things and references to the header comments. > Some people might prefer doxygen exploring the propagation models. Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-los-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:45: DoubleValue (2160e6), On 2012/04/28 14:15:03, Pavel Boyko wrote: > The same as above on default value same reply, see other comment http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:55: NS_LOG_FUNCTION (this); On 2012/04/28 14:15:03, Pavel Boyko wrote: > alignment Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:60: double Lbp = fabs (20 * log10 ((m_lambda * m_lambda) / (8 * M_PI * a->GetPosition ().z * b->GetPosition ().z))); On 2012/04/28 14:15:03, Pavel Boyko wrote: > oops, M_PI here and double pi = 3.1415... in another file. Be consistent. switched to M_PI in the other file > Using M_PI don't forget to define _USE_MATH_DEFINES I see no occurrence of _USE_MATH_DEFINES in the ns-3 codebase. I suggest that eventually we should deal with this as a ns-3 bug. Hence I am leaving this code as is for now. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:82: { On 2012/04/28 14:15:03, Pavel Boyko wrote: > I'd add NS_ASSERT (freq > 0) Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.cc:94: On 2012/04/28 14:15:03, Pavel Boyko wrote: > No tests :( again, test coverage is provided by the buildings module: http://codereview.appspot.com/5728062/patch/1/34 http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-los-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.h:29: On 2012/04/28 14:15:03, Pavel Boyko wrote: > Doxygen comment wanted. Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-los-propagation-loss-model.h:38: /** On 2012/04/28 14:15:03, Pavel Boyko wrote: > The same as above on frequency vs. lambda same reply as above http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:41: On 2012/04/28 05:38:27, Pavel Boyko wrote: > People tend to have explicit getters and setters as well, I am personally > neutral about this. since you're neutral, I'll leave as-is for the time being. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:45: "The Frequency (default is 2.106 GHz).", On 2012/04/28 05:38:27, Pavel Boyko wrote: > Strange default! Nobody will actually know this. The value falls in the downlink 3GPP Band I (a.k.a. the IMT band). I think it's a reasonable choice since Okumura Hata is normally used for cellular systems. Any alternative suggestion? Anyway, the good practice is that every user sets this value according to their scenario. If a user that does not do so, it means he does not care much about the behavior of the propagation model, hence any default frequency that it is in the range of the model will be equally fine. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:121: NS_FATAL_ERROR (this << " Street Orientation must be in [0,90]"); On 2012/04/28 05:38:27, Pavel Boyko wrote: > This is better done by NS_ASSERT since you already check [0, 90] range in the > attribute setter. Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:130: double pi = 3.141592653589793; On 2012/04/28 05:38:27, Pavel Boyko wrote: > Well, if you don't believe to M_PI from <cmath> use const double at least. changed to M_PI http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.cc:223: On 2012/04/28 05:38:27, Pavel Boyko wrote: > A LOT of unit tests should be here or I will never believe that expressions > above are correct. Will try to find tests in separate files. test coverage is provided by the buildings module: http://codereview.appspot.com/5728062/patch/1/34 http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... File src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h:30: On 2012/04/28 05:38:27, Pavel Boyko wrote: > Doxygen class comment is wanted. Don't forget to add \ingroup propagation Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/itu-r-... src/propagation/model/itu-r-1411-nlos-over-rooftop-propagation-loss-model.h:40: * Set the operating frequency On 2012/04/28 05:38:27, Pavel Boyko wrote: > The same comment about labmda as for Hata-Okumura. same reply as there. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... File src/propagation/model/okumura-hata-propagation-loss-model.cc (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:38: OkumuraHataPropagationLossModel::GetTypeId (void) On 2012/04/28 05:38:27, Pavel Boyko wrote: > Explicit setter/getter methods for the attributes can be useful. I don't agree. One can always use okumuraHataPtr->SetAttribute ("Frequency", ...), so why should I bother adding OkumuraHataPropagationLossModel::SetFrequency() if I don't need it for the internal working of OkumuraHataPropagationLossModel? http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:44: .AddAttribute ("Frequency", On 2012/04/28 05:38:27, Pavel Boyko wrote: > This is not consistent with Friis and COST231 Hata propagation loss model, where > carrier frequency is parametrized as Lambda in meters. If the goal is to have a common attribute among all models that support a similar parameter, I'd rather use Frequency than Lambda. It's more user-friendly. Note that Cost231PropagationLossModel has both Lambda and Frequency attributes, but this could be subject to subtle initialization bugs if the user ever tries to change only one of the default values without changing the other. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:77: // standard Okumura Hata (from wikipedia) On 2012/04/28 05:38:27, Pavel Boyko wrote: > I'd personally prefer more "scientific" sources, but ok -- just add URL changed to a "more scientific" reference (the equation is just the same) > and copy > this comment to the header (class comment) The reference is to a specific equation, not to the whole model, hence I think it's too specific for the doxygen description of the class. The equation is already in the .rst documentation, and I've added a note in the doxygen referring to the .rst documentation. I think this is sufficient. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:100: // NS_LOG_INFO (this << " logf " << 26.16 * log_f << " loga " << log_aHeight << " X " << (((44.9 - (6.55 * log10(hb)) ))*log10 (a->GetDistanceFrom (b))) << " logb " << log_bHeight); On 2012/04/28 05:38:27, Pavel Boyko wrote: > Commented source code is bad Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:112: else if (m_frequency <= 2.170e9) // max 3GPP freq EUTRA band #1 On 2012/04/28 05:38:27, Pavel Boyko wrote: > and I am surprised why Hata-Okumura are related to the 3GPP frequency bands? I detached the 2.6 model from OkumuraHataPathlossModel, it's now in the new class Kun2600MhzPropagationLossModel. In this way, the only threshold in OkumuraHataPathlossModel is the one at 1.5 GHz to distinguish between the "standard" OkumuraHata and the enhanced COST231 version. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:138: // "Path Loss Models for Suburban Scenario at 2.3GHz, 2.6GHz and 3.5GHz" On 2012/04/28 05:38:27, Pavel Boyko wrote: > Copy me to the header copied to the header of the new class Kun2600MhzPropagationLossModel. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:143: return loss; On 2012/04/28 05:38:27, Pavel Boyko wrote: > I see no branch for large frequencies, it seems loss = 0 for freq > 2.69GHz! > Will return to this later. Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.cc:154: On 2012/04/28 05:38:27, Pavel Boyko wrote: > Where are the tests? It is generally convenient to have unit tests right below > the tested code. well, tests shouldn't go toghether with the model, but rather in the test directory... Anyway, as said elsewhere in the review, the test coverage is provided by the buildings module, see http://codereview.appspot.com/5728062/patch/1/34 http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... File src/propagation/model/okumura-hata-propagation-loss-model.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.h:30: On 2012/04/28 05:38:27, Pavel Boyko wrote: > Doxygen class comment wanted Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/okumur... src/propagation/model/okumura-hata-propagation-loss-model.h:40: * On 2012/04/28 05:38:27, Pavel Boyko wrote: > Empty lines? Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... File src/propagation/model/propagation-environment.h (right): http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... src/propagation/model/propagation-environment.h:31: * The type of propagation environment On 2012/04/28 05:38:27, Pavel Boyko wrote: > \ingroup propagation Done. http://codereview.appspot.com/5728062/diff/13001/src/propagation/model/propag... src/propagation/model/propagation-environment.h:40: /** On 2012/04/28 05:38:27, Pavel Boyko wrote: > \ingroup propagation Done.
Sign in to reply to this message.
Hi Nicola, I confirm that all comments are addressed in proper way, just move unit tests to the propagation/test/. I'd suggest to have dedicated test suites for different propagation models to keep them completely separated. Frequency / Lambda issue to be addressed in uniform way for all propagation loss models in the next release cycle, this is https://www.nsnam.org/bugzilla/show_bug.cgi?id=1421 now, comment on the bugzilla please. I agree that _USE_MATH_DEFINES issue is out of this review scope. Best regards, Pavel
Sign in to reply to this message.
On 2012/05/02 12:45:26, Pavel Boyko wrote: > I confirm that all comments are addressed in proper way, just move unit tests to > the propagation/test/. I'd suggest to have dedicated test suites for different > propagation models to keep them completely separated. The new patch set 6 does this. Note that the relevant test cases are duplicated from those in the buildings module, rather than moved. The reason is that we still need all test cases in the buildings module to cover properly the functionality of HybridBuildingsPropagationLossModel (in particular, to make sure that its pathloss logic works correctly). Regards, Nicola
Sign in to reply to this message.
Hi Nicola and Marco, Great job! Please address the only comment below and push. Best regards, Pavel http://codereview.appspot.com/5728062/diff/27001/src/propagation/test/okumura... File src/propagation/test/okumura-hata-test-suite.cc (right): http://codereview.appspot.com/5728062/diff/27001/src/propagation/test/okumura... src/propagation/test/okumura-hata-test-suite.cc:118: It is not clear how the reference numbers were obtained (original report? another implementation? manual calculations? other?). Please add an explaining comment. This is related to all test suites here.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
