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?
src/parcels/_core/kernel.py
Outdated
| 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....
There was a problem hiding this comment.
OK, I fixed the issue in ab53284. Instead of making the _positionupdate_kernel_added boolean an attribute of the Kernels, I moved it to the ParticleSet. This makes much more sense logically, so is actually a cleaner solution
This means I could also remove the option that the input to kernel.__init__() is an object in 822db95; it can now only be a list of functions
There was a problem hiding this comment.
Looks good
Though I will say that _positionupdate_kernel_added seems like an anti-pattern to me as it requires setting a flag. What about us providing an attribute _full_kernel that just returns the full thing with the position update kernels attached? I think that would be less error prone
There was a problem hiding this comment.
Yes, I agree that the name _positionupdate_kernel_added is not ideal, so I'll think about a better name. But I don't really see how this can be done without a flag. I tried a bit with a _full_kernel attribute, but can't get it to work. What kind of logic were you thinking of? Can you elaborate?
src/parcels/_core/particleset.py
Outdated
| 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 |
This means that pset executions can also be run in a loop without a kernel object; see the discussion at Parcels-code#2490 (comment)
VeckoTheGecko
left a comment
VeckoTheGecko
left a comment
There was a problem hiding this comment.
#2490 (comment) is my final review comment here. Happy to merge (either after that's fixed, or can implement #2490 (comment) in a separate PR)
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