Skip to content
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

Try to improve inference #593

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Try to improve inference #593

merged 1 commit into from
Nov 30, 2021

Conversation

charleskawczynski
Copy link
Member

This PR tries to improve inference by improving inference, and fixes some variable names to conform to our naming conventions.

@charleskawczynski
Copy link
Member Author

bors r+

@jakebolewski
Copy link

jakebolewski commented Nov 30, 2021

It would be better to test these with JET, Val can be useful but has a large compile time cost. If you are using parameterized Val{symbol} for dispatch / specialization then it might be cleaner to refactor them into types.

@charleskawczynski
Copy link
Member Author

It would be better to test these with JET, Val can be useful but has a large compile time cost. If you are using parameterized Val{symbol} for dispatch / specialization then it might be cleaner to refactor them into types.

Yeah, I agree that it'd be ideal to use JET / unit tests, I figured we'll wait until 1.7 is released and we can incorporate it into our CI.

I also agree about avoiding the use of Val, it's just the simplest thing for now (to see if these are in fact the source of allocations) since there's not a ton of combinations, and we're using these symbols with getproperty.

@bors
Copy link
Contributor

bors bot commented Nov 30, 2021

Build succeeded:

@bors bors bot merged commit 6319636 into main Nov 30, 2021
@bors bors bot deleted the ck/inference branch November 30, 2021 23:53
charleskawczynski referenced this pull request Dec 1, 2021
598: Bump version number r=charleskawczynski a=charleskawczynski



Co-authored-by: Charles Kawczynski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants