Adds generic log to the math dialect#633
Conversation
|
When I ran _gen.py under the math dialect, it edited what were formerly references to "lowering" to "lowering2" - it seems to have been an intentional change based on the commit history, but it doesn't seem to be properly recognized, so I also wanted to verify if this was intended or should be kept as lowering? |
@aadi07 I think this need to update too. there should be only "lowering" exists now. |
|
Got it, that change should be reflected now as well. Does everything else look good? |
src/kirin/dialects/math/__init__.py
Outdated
| @lowering.wraps(stmts.trunc) | ||
| def trunc(x: float) -> int: ... | ||
|
|
||
| def trunc(x: int) -> int: ... |
src/kirin/dialects/math/interp.py
Outdated
| @impl(stmts.fma) | ||
| def fma(self, interp, frame: Frame, stmt: stmts.fma): | ||
| values = frame.get_values(stmt.args) | ||
| return (math.fma(values[0], values[1], values[2]),) | ||
|
|
There was a problem hiding this comment.
please remove fma from this PR. it only support on 3.13+
|
I think some of them are because of version. Some of them are just can be
implemented with subroutine (for example factorial). If you want to keep it
clean in this PR, let's add fma to that list at least
…On Sun, Apr 5, 2026, 19:10 Advaith Anand ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/kirin/dialects/math/__init__.py
<#633 (comment)>:
> @lowering.wraps(stmts.floor)
-def floor(x: float) -> int: ...
+def floor(x: int) -> int: ...
I also noticed that the list of exempted functions at the top of _gen.py,
aside from the ones that are exempted due to a version issue, seem to
exempted due to having integer inputs. If the only reason these weren't
include is an inconvenience in generalizing their signatures, I could
include them now?
# skip some special cases for now
if name in (
"prod",
"perm",
"modf",
"ldexp",
"lcm",
"isqrt",
"isclose",
"gcd",
"fsum",
"frexp",
"factorial",
"acosh",
"comb",
"dist",
"sumprod",
"nextafter",
# 3.10 compat
"cbrt",
"exp2",
# 3.13 compat
"fma",
):
—
Reply to this email directly, view it on GitHub
<#633 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFCX3SPUR2NLEBRVBK7ZTKD4ULRYBAVCNFSM6AAAAACXMLWN7KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANJZHEZDQOJTGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I think the latest commit should have resolved this. I've added fma to the excepted functions list, and made sure that all input parameters are encoded as floats by default, with only the outputs varying appropriately. |
|
@aadi07 can you fix the linting issue? you can use pre-commit tool to do that for you |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Fixes #131 - however the issue was caused by inspect.signature throwing a value error when the function passed in has variable arguments (in the case of math.log, the base argument is optional). I've bypassed this by forcing the log to have a base passed in. Not sure this is ideal for the use case here (could be preferable to default to the natural log). Open to any suggestions.