Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| class LineTaylorLocalExpansion(LocalExpansionBase): | ||
| def __init__(self, kernel, order, tau=1, use_rscale=None, m2l_translation=None): | ||
| super().__init__(kernel, order, use_rscale, m2l_translation) | ||
| self.tau = tau |
There was a problem hiding this comment.
[P1] Include τ in LineTaylor expansion identity
The constructor now stores a tau parameter, but LineTaylorLocalExpansion still relies on LocalExpansionBase’s default init_arg_names, __eq__, and persistent-hash implementation, which only look at the kernel, order, and use_rscale. As a result, two expansions constructed with different τ values compare equal and generate identical cache keys, so enabling kernel caching (or calling copy()/with_kernel()) will reuse code compiled with the wrong τ and silently produce incorrect matrices. Please add τ to the expansion’s init args and hashing/equality so that different τ values generate distinct kernels and survive copy().
Useful? React with 👍 / 👎.
| def __init__(self, | ||
| kernel, | ||
| asymptotic, | ||
| order, | ||
| tau=1, | ||
| use_rscale=None, | ||
| m2l_translation=None): | ||
| super().__init__(kernel, order, use_rscale, m2l_translation) | ||
| self.asymptotic = asymptotic | ||
| self.tau = tau |
There was a problem hiding this comment.
[P1] Distinguish asymptotic line expansions by asymptotic/tau
The new AsymptoticDividingLineTaylorExpansion captures both asymptotic and tau, yet the class inherits __eq__/persistent hashing from LocalExpansionBase, which ignore these attributes. When the kernel cache or copy() is used, two expansions with different asymptotic functions or τ values will be treated as identical and the first compiled kernel will be reused for the second, yielding coefficients for the wrong asymptotic form. Include asymptotic and tau in the expansion’s init arg list and persistent hash/equality so cache keys reflect these parameters.
Useful? React with 👍 / 👎.
Add AsymptoticDividingLineTaylorExpansion class, todo: add test case documentaions + autoclass + rename
inducer
force-pushed
the
feature/asymline
branch
from
4207c9b to
9ebc1af
Compare
2 participants
parallel to inducer#233