bwinkel / pycraf

pycraf is a package that provides functions and procedures for various tasks in spectrum-management compatibility studies.
35 stars 15 forks source link

Conversions Module arithmetic operator precedence issue #21

Open cbowers1020 opened 4 years ago

cbowers1020 commented 4 years ago

In the conversions module file conversions.py, there are many examples of using multiple division operators to achieve the final form of an equation (e.g., freespace loss).

In the case of the free_space_loss calculation (method: _free_space_loss), (C_VALUE / 4. / np.pi / f / d) 2 !=* (4 np.pi f d) / C_VALUE) 2 (a more human readable format. Though the pycraf implementation ((C_VALUE / 4. / np.pi / f / d) 2) is mathematically correct, python does NOT evaluate the operators correctly. I have remedied this error locally, but I suspect (though have not verified) that the following methods in conversions.py are additionally affected by this mathematical implementation:

This error does not affect any pyprop implementations and free space loss using that method reflects the actual calculation.

bwinkel commented 4 years ago

Thanks a lot for your interest in pycraf!

I'm not sure, if I get your point, especially when you write that

python does NOT evaluate the operators correctly

Could you perhaps add an example of what gets incorrectly calculated?

cbowers1020 commented 4 years ago

Sure. After closer inspection, I think I overstated the error. The actual problem is as follows.

In calculating free space path loss (which is how I found this error), the equation would be: FSPL = ((4. pi distance * frequency) / C_Constant) ^ 2

This equation is implemented in conversions.py in method _free_space_loss as: pycraf_FSPL = (C_Constant / 4. / pi / frequency / distance) ** 2, which evaluates to 1 / FSPL. I guess this is not a problem, but this value when returned to the wrapper function, free_space_loss, is treated as the regular, what I call, FSPL value.

Additionally, though this doesn't affect the calculation, the wrapper method free_space_loss switches frequency and distance when passing them into the implementation method _free_space_loss.

I hope this explanation makes a bit more sense. I apologize for the earlier confusion.

Screen Shot 2020-02-20 at 7 06 12 PM
bwinkel commented 4 years ago

I see. Most RF engineers who I know prefer the FSPL (expressed in dB) to be a positive number (as it is a loss), whereas one can also find the definition, which you quote, where it is a negative decibel value. Personally, I have no strong feelings about this, but as the ITU-R recommendations, e.g., P.452, which are used in pycraf also follow the former definition, I'll leave the free_space_loss function as it is.

I'm sorry about the switch of frequency and distance in the function definition, which is very unfortunate indeed. I may fix this in a future release, although this is dangerous - if someone made use of the _free_space_loss in their software, it would break their code.

cbowers1020 commented 4 years ago

That's actually how I came upon this error--I was expecting a positive loss value and received a negative value.

Only for low frequencies or very short distances will the original pycraf implementation return a value greater than one and thus, the conversion to dB will result in a positive number.

Below is a sampling of calculationa in the wireless network I am simulating at various frequencies: 5GHz, 5MHz, and 5000 Hz.

I guess we are both ultimately dancing around the same value, so it doesn't really matter. With this equation though, (versus the pyprof free space loss calculation implementation: 92.5 + 20 log10(frequency) + 20 log10(distance) for GHz and km), it would be hard for the user to know what sign to expect.

Screen Shot 2020-02-21 at 7 28 33 AM Screen Shot 2020-02-21 at 7 31 55 AM Screen Shot 2020-02-21 at 7 32 09 AM
bwinkel commented 4 years ago

I see. Then I defined it in the opposite way. I'll probably fix this in a future release, although it is always a bit painful for downstream developers if you change the API.