-
Notifications
You must be signed in to change notification settings - Fork 171
Renaming pyfunc to kernels #2490
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
Changes from 6 commits
4b1e4e6
2497376
5fb6178
b1e2e2f
0bf0e42
d9c770d
1884adb
ab53284
822db95
d18c7d6
516ed9d
1c92b9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,6 @@ | |
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
|
|
||
| __all__ = ["Kernel"] | ||
|
|
||
|
|
||
| ErrorsToThrow = { | ||
| StatusCode.ErrorOutsideTimeInterval: _raise_outside_time_interval_error, | ||
|
|
@@ -45,12 +43,12 @@ class Kernel: | |
|
|
||
| Parameters | ||
| ---------- | ||
| kernels : | ||
| list of Kernel functions | ||
| fieldset : parcels.Fieldset | ||
| FieldSet object providing the field information (possibly None) | ||
| ptype : | ||
| PType object for the kernel particle | ||
| pyfunc : | ||
| (aggregated) Kernel function | ||
|
|
||
| Notes | ||
| ----- | ||
|
|
@@ -60,32 +58,37 @@ class Kernel: | |
|
|
||
| def __init__( | ||
| self, | ||
| kernels: list[types.FunctionType], | ||
| fieldset, | ||
| ptype, | ||
| pyfuncs: list[types.FunctionType], | ||
| ): | ||
| for f in pyfuncs: | ||
| if not isinstance(f, types.FunctionType): | ||
| raise TypeError(f"Argument pyfunc should be a function or list of functions. Got {type(f)}") | ||
| if not isinstance(kernels, list): | ||
| raise ValueError(f"kernels must be a list. Got {kernels=!r}") | ||
|
|
||
|
erikvansebille marked this conversation as resolved.
|
||
| for f in kernels: | ||
| if isinstance(f, Kernel): | ||
| f = f._kernels # unwrap | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? Not sure why this was added - slipped by me during the last review
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The point is that there is a difference between a function and a Kernel, because the latter has the 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....
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I fixed the issue in ab53284. Instead of making the This means I could also remove the option that the input to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good Though I will say that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree that the name |
||
| elif not isinstance(f, types.FunctionType): | ||
| raise TypeError(f"Argument `kernels` should be a function or list of functions. Got {type(f)}") | ||
| assert_same_function_signature(f, ref=AdvectionRK4, context="Kernel") | ||
|
|
||
| if len(pyfuncs) == 0: | ||
| raise ValueError("List of `pyfuncs` should have at least one function.") | ||
| if len(kernels) == 0: | ||
| raise ValueError("List of `kernels` should have at least one function.") | ||
|
|
||
| self._fieldset = fieldset | ||
| self._ptype = ptype | ||
|
|
||
| self._positionupdate_kernel_added = False | ||
|
|
||
| for f in pyfuncs: | ||
| for f in kernels: | ||
| self.check_fieldsets_in_kernels(f) | ||
|
|
||
| self._pyfuncs: list[Callable] = pyfuncs | ||
| self._kernels: list[Callable] = kernels | ||
|
|
||
| @property #! Ported from v3. To be removed in v4? (/find another way to name kernels in output file) | ||
| def funcname(self): | ||
| ret = "" | ||
| for f in self._pyfuncs: | ||
| for f in self._kernels: | ||
| ret += f.__name__ | ||
| return ret | ||
|
|
||
|
|
@@ -123,21 +126,21 @@ def PositionUpdate(particles, fieldset): # pragma: no cover | |
| # Update dt in case it's increased in RK45 kernel | ||
| particles.dt = particles.next_dt | ||
|
|
||
| self._pyfuncs = (PositionUpdate + self)._pyfuncs | ||
| self._kernels = [PositionUpdate] + self._kernels | ||
|
|
||
| def check_fieldsets_in_kernels(self, pyfunc): # TODO v4: this can go into another method? assert_is_compatible()? | ||
| def check_fieldsets_in_kernels(self, kernel): # TODO v4: this can go into another method? assert_is_compatible()? | ||
| """ | ||
| Checks the integrity of the fieldset with the kernels. | ||
|
|
||
| This function is to be called from the derived class when setting up the 'pyfunc'. | ||
| This function is to be called from the derived class when setting up the 'kernel'. | ||
| """ | ||
| if self.fieldset is not None: | ||
| if pyfunc is AdvectionAnalytical: | ||
| if kernel is AdvectionAnalytical: | ||
| if self._fieldset.U.interp_method != "cgrid_velocity": | ||
| raise NotImplementedError("Analytical Advection only works with C-grids") | ||
| if self._fieldset.U.grid._gtype not in [GridType.CurvilinearZGrid, GridType.RectilinearZGrid]: | ||
| raise NotImplementedError("Analytical Advection only works with Z-grids in the vertical") | ||
| elif pyfunc is AdvectionRK45: | ||
| elif kernel is AdvectionRK45: | ||
| if "next_dt" not in [v.name for v in self.ptype.variables]: | ||
| raise ValueError('ParticleClass requires a "next_dt" for AdvectionRK45 Kernel.') | ||
| if not hasattr(self.fieldset, "RK45_tol"): | ||
|
|
@@ -174,48 +177,11 @@ def merge(self, kernel): | |
| assert self.ptype == kernel.ptype, "Cannot merge kernels with different particle types" | ||
|
|
||
| return type(self)( | ||
| self._kernels + kernel._kernels, | ||
| self.fieldset, | ||
| self.ptype, | ||
| pyfuncs=self._pyfuncs + kernel._pyfuncs, | ||
| ) | ||
|
|
||
| def __add__(self, kernel): | ||
| if isinstance(kernel, types.FunctionType): | ||
| kernel = type(self)(self.fieldset, self.ptype, pyfuncs=[kernel]) | ||
| return self.merge(kernel) | ||
|
|
||
| def __radd__(self, kernel): | ||
| if isinstance(kernel, types.FunctionType): | ||
| kernel = type(self)(self.fieldset, self.ptype, pyfuncs=[kernel]) | ||
| return kernel.merge(self) | ||
|
|
||
| @classmethod | ||
| def from_list(cls, fieldset, ptype, pyfunc_list): | ||
| """Create a combined kernel from a list of functions. | ||
|
|
||
| Takes a list of functions, converts them to kernels, and joins them | ||
| together. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| fieldset : parcels.Fieldset | ||
| FieldSet object providing the field information (possibly None) | ||
| ptype : | ||
| PType object for the kernel particle | ||
| pyfunc_list : list of functions | ||
| List of functions to be combined into a single kernel. | ||
| *args : | ||
| Additional arguments passed to first kernel during construction. | ||
| **kwargs : | ||
| Additional keyword arguments passed to first kernel during construction. | ||
| """ | ||
| if not isinstance(pyfunc_list, list): | ||
| raise TypeError(f"Argument `pyfunc_list` should be a list of functions. Got {type(pyfunc_list)}") | ||
| if not all([isinstance(f, types.FunctionType) for f in pyfunc_list]): | ||
| raise ValueError("Argument `pyfunc_list` should be a list of functions.") | ||
|
|
||
| return cls(fieldset, ptype, pyfunc_list) | ||
|
|
||
| def execute(self, pset, endtime, dt): | ||
| """Execute this Kernel over a ParticleSet for several timesteps. | ||
|
|
||
|
|
@@ -248,7 +214,7 @@ def execute(self, pset, endtime, dt): | |
| pset.dt = np.minimum(np.maximum(pset.dt, -time_to_endtime), 0) | ||
|
|
||
| # run kernels for all particles that need to be evaluated | ||
| for f in self._pyfuncs: | ||
| for f in self._kernels: | ||
| f(pset[evaluate_particles], self._fieldset) | ||
|
|
||
| # check for particles that have to be repeated | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.