Renaming pyfunc to kernels #2490
Conversation
src/parcels/_core/kernel.py
Outdated
| def __add__(self, kernel): | ||
| if isinstance(kernel, types.FunctionType): | ||
| kernel = type(self)(self.fieldset, self.ptype, pyfuncs=[kernel]) | ||
| kernel = type(self)([kernel], self.fieldset, self.ptype) | ||
| return self.merge(kernel) | ||
|
|
||
| def __radd__(self, kernel): | ||
| if isinstance(kernel, types.FunctionType): | ||
| kernel = type(self)(self.fieldset, self.ptype, pyfuncs=[kernel]) | ||
| kernel = type(self)([kernel], self.fieldset, self.ptype) | ||
| return kernel.merge(self) |
There was a problem hiding this comment.
Not for this PR - but what functionality does the merging of kernels have in version 4 of Parcels? Are there any times users (or us internally) would want to merge kernels?
| if isinstance(f, Kernel): | ||
| f = f._kernels # unwrap |
There was a problem hiding this comment.
? Not sure why this was added - slipped by me during the last review
There was a problem hiding this comment.
No, this is an addition after your reviews. We need this for one specific unit test (below) where a set.execute() is called in a for-loop
https://github.com/erikvansebille/Parcels/blob/32bcf937cb3776d3ac177d992ccf698de435954a/tests/test_particleset_execute.py#L253-L256
The point is that there is a difference between a function and a Kernel, because the latter has the positionupdatekernel added.
https://github.com/erikvansebille/Parcels/blob/32bcf937cb3776d3ac177d992ccf698de435954a/src/parcels/_core/kernel.py#L283-L287
If a user would provide the function in the for-loop above, then the code above is triggered on each iteration, while if they provide the kernel object it's only triggered the first time
This difference between calling pest.execute with a function or a Kernel object may cause confusion to users, I now realise. But the code at the end of the kernel-loop is essential to make the updating of variables correct; this has been a lot of headaches in #2333, so I'm reluctant to change this again.
Something to think about when we're back from holidays....
| if isinstance(kernels, Kernel): | ||
| self._kernel = kernels | ||
| else: | ||
| if not isinstance(kernels, list): | ||
| kernels = [kernels] | ||
| self._kernel = Kernel(kernels, self.fieldset, self._ptype) |
There was a problem hiding this comment.
kernels input arg can't be Kernel object anymore. Also inverting the isinstance check would be more reliable
| if isinstance(kernels, Kernel): | |
| self._kernel = kernels | |
| else: | |
| if not isinstance(kernels, list): | |
| kernels = [kernels] | |
| self._kernel = Kernel(kernels, self.fieldset, self._ptype) | |
| if isinstance(kernels, types.FunctionType): | |
| kernels = [kernels] | |
| self._kernel = Kernel(kernels, self.fieldset, self._ptype) |
suggestion requires import types at the top of the file
There was a problem hiding this comment.
This is related to the comment above #2490 (comment); there is a use-case for still accepting Kernels as input. unless we find another way to figure out whether the position_update_kernel has to be added or not
|
I think we can merge after those two comments above are addressed |
2 participants
This PR implements #2275 by renaming the
pyfuncargument tokernels.It also simplifies the pset.execute() by assuming that the input is a list of functions, rather than Kernel objects. This means that
pset.Kernelis not needed anymore, and we can get rid of a few helper functions, decluttering the code