Skip to content

Conversation

@devmotion
Copy link
Member

@devmotion devmotion commented Mar 8, 2022

Fixes #393.

Well, it fixes the error but evaluation does not work it seems.

cc: @torfjelde

Copy link
Member

@yebai yebai 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 - thanks, @devmotion.

@yebai
Copy link
Member

yebai commented Mar 9, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 9, 2022
~~Fixes #393

Well, it fixes the error but evaluation does not work it seems.
@yebai
Copy link
Member

yebai commented Mar 9, 2022

bors cancel

@bors
Copy link
Contributor

bors bot commented Mar 9, 2022

Canceled.

@yebai
Copy link
Member

yebai commented Mar 9, 2022

bors try

bors bot added a commit that referenced this pull request Mar 9, 2022
@devmotion
Copy link
Member Author

The PR doesn't work (yet) - when constructing the model in the example it will store ispredict::Bool instead of a Val.

@bors
Copy link
Contributor

bors bot commented Mar 9, 2022

try

Build failed:

@devmotion
Copy link
Member Author

I think we might have to change our handling of arguments a bit and just store unnamed arguments with a gensymed name in the Model. There is no clear mapping between static type parameters and arguments, so I assume (haven't checked yet) it's easy to come up with other bug reports. (I also think it's easy to break stuff with kwargs and provoke undesired method redefinitions, so I assume we should keep positional and keyword arguments separate in the Model).

@yebai
Copy link
Member

yebai commented Mar 9, 2022

change our handling of arguments a bit and just store unnamed arguments with a gensymed name in the Model.

That sounds like a sensible plan. More generally, I think we should aim for a more modular, less magical implementation of the @mode macro. It might be a good idea to open a new issue, and list some desirable objectives of a "perfect" model macro.

cc @rikhuijzer

@devmotion
Copy link
Member Author

devmotion commented Mar 9, 2022

Arguably it's already quite simple and non-magical, apart from these unnamed arguments. It definitely became much much simpler already. There's not many transformations of the user-specified functions anymore, we just replace the tilde statements (there are separate functions for undotted and dotted tildes) and add our internal arguments. IMO the complexity is mainly/only due to the runtime dispatches in the context pipeline (which is separate from @model).

@devmotion
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Mar 9, 2022
@devmotion devmotion requested a review from yebai March 9, 2022 21:12
@bors
Copy link
Contributor

bors bot commented Mar 9, 2022

try

Build failed:

@devmotion
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 9, 2022
Fixes #393.

~~Well, it fixes the error but evaluation does not work it seems.~~

cc: @torfjelde
@bors
Copy link
Contributor

bors bot commented Mar 10, 2022

@bors bors bot changed the title Fix anonymous argument with type parameter [Merged by Bors] - Fix anonymous argument with type parameter Mar 10, 2022
@bors bors bot closed this Mar 10, 2022
@bors bors bot deleted the dw/anonymous_typeparameter branch March 10, 2022 00:36
@torfjelde
Copy link
Member

Awesome stuff @devmotion ❤️

@phipsgabler
Copy link
Member

Uh, finally someone doing this :D Thanks @devmotion, where would be be without you...

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.

Anonymous arguments aren't handled properly

5 participants