Skip to content

Comments

Improve kernel type annotations#242

Closed
alexfikl wants to merge 11 commits intoinducer:dataclass-kernelfrom
alexfikl:dataclass-kernel-typing
Closed

Improve kernel type annotations#242
alexfikl wants to merge 11 commits intoinducer:dataclass-kernelfrom
alexfikl:dataclass-kernel-typing

Conversation

@alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Sep 9, 2025

This is work on top of #240 that adds more types and improves some docs.

I mostly wanted to type the derivative removers for some stuff in pytential, but started adding more.. and then realized you're already working on it in #240. Hopefully this isn't interfering too much 😟

# {{{ DifferentiatedExprDerivativeTaker

DerivativeCoeffDict = dict[tuple[int, ...], Any]
DerivativeCoeffDict = dict[tuple[int, ...], int | float | complex | sym.Expr]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure this is correct, but it seems to match most usage?

# Before adding a function here, make sure it's present in both modules.
Add = sym.Add
Basic = sym.Basic
Expr = sym.Expr
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mostly added this for typing. Not sure it's a good idea?

@alexfikl alexfikl force-pushed the dataclass-kernel-typing branch from 91f34c1 to 983fff0 Compare September 9, 2025 13:39
Comment on lines -304 to 414

def __repr__(self):
@override
def __str__(self) -> str:
return f"ExprKnl{self.dim}D"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seemed more of a __str__ than __repr__. Is that ok? I think some of the derivative wrappers already just defined __str__ like this.

helmholtz_k_name: str
"""The argument name to use for the Helmholtz parameter when generating
functions to evaluate this kernel.
"""
Copy link
Collaborator Author

@alexfikl alexfikl Sep 9, 2025

Choose a reason for hiding this comment

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

Is allow_evanescent actually used anywhere? Couldn't find what it's for..

def get_derivative_taker(
self,
dvec: sp.Matrix,
rscale: ArithmeticExpr,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a good type for rscale? Most places I could find just set it to 1 (or some level dependent float?).

@alexfikl alexfikl force-pushed the dataclass-kernel-typing branch 2 times, most recently from 9c8fe87 to 111794c Compare September 9, 2025 14:07
@alexfikl alexfikl marked this pull request as ready for review September 9, 2025 14:07
@alexfikl
Copy link
Collaborator Author

alexfikl commented Sep 9, 2025

This should mostly have the same failures as #240, minus the pyright errors, which were rolled into the baseline (as far as I can tell, since I didn't fix anything there 😁).

@alexfikl alexfikl force-pushed the dataclass-kernel-typing branch from 111794c to e267a48 Compare September 9, 2025 14:14
@inducer
Copy link
Owner

inducer commented Sep 9, 2025

Hopefully this isn't interfering too much 😟

No worries at all! What's left for #240 is to fix failures. Would you still like me to finish that out and then rebase this?

@inducer
Copy link
Owner

inducer commented Sep 9, 2025

Ah nvm, you just answered that. :)

@alexfikl
Copy link
Collaborator Author

alexfikl commented Sep 9, 2025

Ah nvm, you just answered that. :)

Yeah 😁 I looked at those StokesletKernel things, but I wasn't sure how to cleanly fix it. This apparently has some additional failures I managed to introduce, so I'll look into fixing those.

@alexfikl alexfikl force-pushed the dataclass-kernel-typing branch 2 times, most recently from 52769d7 to eae30ab Compare September 9, 2025 17:26
@alexfikl alexfikl force-pushed the dataclass-kernel-typing branch from eae30ab to 812e1d0 Compare September 9, 2025 17:40
@alexfikl
Copy link
Collaborator Author

alexfikl commented Sep 9, 2025

@inducer This looks good to go from my end (only the Stokeslet errors from #240 seem to remain). Let me know if any of the annotations look fishy.

@inducer inducer deleted the branch inducer:dataclass-kernel September 9, 2025 20:59
@inducer inducer closed this Sep 9, 2025
@inducer
Copy link
Owner

inducer commented Sep 9, 2025

Argh. It looks like merging #240 autoclosed this. That was not the intent. Github, are you OK?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants