Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support use of parcels.rng in Kernels #1618

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

erikvansebille
Copy link
Member

This PR fixes #1608. it supports using parcels.rng directly in Kernels (note, parcels.ParcelsRandom is also supported, but since the module itself it called rng better to use parcels.rng for traceability)

@erikvansebille erikvansebille changed the base branch from master to v/update-notebooks July 26, 2024 13:24
@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Jul 26, 2024

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.

@erikvansebille
Copy link
Member Author

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.

Yes I agree that we have to be careful here. Perhaps we should then also change ParcelsRandom -> parcels.rng throughout the tutorials and examples (while keeping support for ParcelsRandom at least until v4). That would also align with #1581

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. test_explicit_ParcelsRandom (i.e., from parcels import ParcelsRandom)
  2. test_parcels_dot_ParcelsRandom (i.e., using parcels.ParcelsRandom)
  3. test_parcels_dot_rng (i.e., using parcels.rng)
  4. test_custom_ParcelsRandom_alias (i.e., from parcels import ParcelsRandom as my_custom_name)
  5. 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.

parcels/kernel.py Show resolved Hide resolved
Based on the patch proposed by @VeckoTheGecko
@erikvansebille
Copy link
Member Author

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 rng. This is handled in https://github.com/OceanParcels/parcels/blob/b12b3abd8bc2f150ac9ee60b5b543cd2c4df70ed/parcels/kernel.py#L193

I prefer to move to parcels.rng throughout the codebase (which means a bit more changes, but clarifies that this is associated with the parcels/rng.py file)

@VeckoTheGecko VeckoTheGecko self-requested a review July 29, 2024 13:06
@erikvansebille erikvansebille merged commit 804ef58 into v/update-notebooks Jul 29, 2024
4 checks passed
@erikvansebille erikvansebille deleted the support_parcels_modules_in_JIT branch July 29, 2024 13:10
VeckoTheGecko added a commit that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Kernel can't be converted to C depending on method of import
2 participants