Conversation
|
Dear @fgiovanard, thanks a lot for your first Pull Request! There a a few minor things, which we'll need to fix before merging; see comments. A general remark, please make a few changes to the formatting. I like to stick to pep8 as much as possible. If you want a quick fix, simply use black to reformat it automatically. I think, you could also simply use this online tool for reformatting. Let me know if you need help. |
|
I forgot to say, please also add a test into the tests subdirectory. (There are plenty of examples about how it's done.) |
pycraf/pathprof/propagation_p368.py
Outdated
|
|
||
| # Data Lookup Function | ||
| def findE40(freq, key_ground_term): | ||
| """ |
There was a problem hiding this comment.
Docstrings require a slightly different format (numpy style). See other functions in pycraf as a reference.
pycraf/pathprof/propagation_p368.py
Outdated
| from astropy import units as u | ||
| import scipy.constants | ||
| from pycraf import protection | ||
|
|
There was a problem hiding this comment.
In order for the functions in this module to appear in pycraf, it is required to put them into the so-called slots (__all__ = ['findE40', ...] list at the top of the file. See other pycraf modules. Furthermore, in the __init__.py file in the pathprof directory, you need to add from .propagation_p368 import *.
pycraf/pathprof/propagation_p368.py
Outdated
| } | ||
|
|
||
| # Ground types with their respective conductivity and permittivity values | ||
| Ground_Types = { |
There was a problem hiding this comment.
Perhaps it would be cleaner to just have the values for sigma and epsilon (stored as integers) instead of the strings? Could add a comment, what they are.
pycraf/pathprof/propagation_p368.py
Outdated
| MIN_VAL, MAX_VAL = 10 * u.kHz, 30 * u.MHz | ||
|
|
||
| # Check if the frequency is within the allowed range | ||
| if not (MIN_VAL <= freq <= MAX_VAL): |
There was a problem hiding this comment.
If you want to make sure that input parameters have a certain value range, you should instead use the @utils.ranged_quantity_input decorator. It also checks the units and applies automatic conversion, if required. It also means that you can work without units in the function core, which may be a bit faster. See other modules, how it's done.
pycraf/pathprof/propagation_p368.py
Outdated
| # Compute the ground-wave propagation distance | ||
| distance_gw = (1000 * np.power(10, (Eint.value - E_limit.to_value(cnv.dB_uV_m)) / 40) * u.m).to(u.km) | ||
|
|
||
| return m_type, d_transition, distance_gw |
There was a problem hiding this comment.
I would probably put the following at the end of the file:
if __name__ == '__main__':
print('This not a standalone python program! Use as module.')
bwinkel
force-pushed
the
master
branch
2 times, most recently
from
d40b599 to
da95291
Compare
2 participants
No description provided.