Support use of parcels.rng in Kernels #1618
Conversation
|
Have managed to very briefly look at this before EOD. If possible, I would like to make kernel behaviour completely robust to how kernel functions are imported (i.e., similar to how Python functions work). As such, I'm apprehensive to do string manipulations on contents of the kernel functions, but instead see if we can look up the location of these functions to verify they're allowed. If such robustness isn't possible, we at least need to have complete compatibility with prior documentation to avoid regressing and breaking existing simulation code. I'll investigate possibilities more next week. |
Yes I agree that we have to be careful here. Perhaps we should then also change |
VeckoTheGecko
left a comment
There was a problem hiding this comment.
Ok, I've managed to look through this fully now. I have also written some test cases (see
changes.patch; do git apply changes.patch to bring them into your working directory), so I'll refer to those hereon.
test_explicit_ParcelsRandom(i.e.,from parcels import ParcelsRandom)test_parcels_dot_ParcelsRandom(i.e., usingparcels.ParcelsRandom)test_parcels_dot_rng(i.e., usingparcels.rng)test_custom_ParcelsRandom_alias(i.e.,from parcels import ParcelsRandom as my_custom_name)- Doing direct function import (i.e.,
from parcels.rng import normalvariate). No tests written.
(1) was the only supported implementation before, and doing any other sort of import broke the kernel conversion code. #1608 is requiring us to extend functionality to (2) as well. Ideally, it would be Pythonic to support all 1-5 so that users don't get surprised, but looking into the code conversion code it doesn't look feasible/worth it.
The invention of ParcelsRandom as an aliased module in itself (instead of using parcels.rng) I'm happy with as it highlights these RNGs are parcels specific, and also because this is familiar to users. Maybe in a V4, if there is already significant API changes, we can rename rng.py to parcels_random.py and then remove the alias, but that's a Python convention nitpick. I'm not sure what you mean by "traceability", so maybe you could elaborate there.
In the meanwhile, I don't mind only supporting ParcelsRandom (hence (3) shouldn't work; which is reflected in the test case I wrote). (4) and (5) shouldn't work due to limitations.
Based on the patch proposed by @VeckoTheGecko
|
Thanks for the patch, @VeckoTheGecko. I adjusted it a bit (adding an assert so that we can check that the random number is indeed added as expected) and pushed it in b12b3ab Note that your third test does work, because we do import I prefer to move to |
This PR fixes #1608. it supports using
parcels.rngdirectly in Kernels (note,parcels.ParcelsRandomis also supported, but since the module itself it calledrngbetter to useparcels.rngfor traceability)