Skip to content

Conversation

@IvanGrigorik
Copy link
Collaborator

This PR addresses #342.

Fixed type inference for functions, so now no need to specify argument types.

@IvanGrigorik IvanGrigorik requested a review from kennykos January 5, 2026 22:33
@IvanGrigorik
Copy link
Collaborator Author

@kennykos can you please check and confirm that it fixes #342?

You have another problem with this line.
Pykokkos requires specifying the types of new values, since autotype inference for all values can lead to some problems (we can fix it in the future, but this PR addresses only the argument inference part.

@kennykos
Copy link
Collaborator

kennykos commented Jan 5, 2026

@kennykos can you please check and confirm that it fixes #342?

You have another problem with this line. Pykokkos requires specifying the types of new values, since autotype inference for all values can lead to some problems (we can fix it in the future, but this PR addresses only the argument inference part.

This fixes #342, given my caveat above about the mangled error message when the return type is not specified.

@kennykos kennykos linked an issue Jan 6, 2026 that may be closed by this pull request
@kennykos
Copy link
Collaborator

kennykos commented Jan 6, 2026

@IvanGrigorik the most recent push checks argument inference for nested @pk.functions; the tests fail.

Is this something we can support as well.

@IvanGrigorik IvanGrigorik requested a review from kennykos January 6, 2026 21:46
@IvanGrigorik
Copy link
Collaborator Author

@kennykos everything is fixed
There was some problems with paths and default arguments like wid, acc, etc

Copy link
Collaborator

@kennykos kennykos left a comment

Choose a reason for hiding this comment

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

I have some questions

  1. Why are we explicitly casting common thread ID names to int?
  2. Why are we casting ints to doubles?
  3. Should we consider other module types in addition to numpy?

See the specific comments for details.

Otherwise, this looks good to me.

@IvanGrigorik
Copy link
Collaborator Author

For now pykokkos View type is supported, but eventually we will deprecate them as well

@IvanGrigorik IvanGrigorik requested a review from kennykos January 7, 2026 21:46

if not types_changed:
break

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have some sort of failure warning here if the iterations max out and there are still uninferred types?

Copy link
Collaborator

@kennykos kennykos left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left one comment about an error message that could be nice, but I'll defer to @IvanGrigorik on that one. Otherwise, looks good to me!

Copy link
Collaborator

@kennykos kennykos left a comment

Choose a reason for hiding this comment

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

Looks great!

@IvanGrigorik IvanGrigorik force-pushed the grigorik/pkfunctions-annotation branch from 91cd766 to 4d2c2a4 Compare January 7, 2026 22:09
@IvanGrigorik IvanGrigorik merged commit 60b8596 into main Jan 7, 2026
22 checks passed
@IvanGrigorik IvanGrigorik deleted the grigorik/pkfunctions-annotation branch January 7, 2026 22:51
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.

[error] pykokkos function breaks with undiscernible traceback

3 participants