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

Slightly generalize _compute_sparam elision #48144

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Slightly generalize _compute_sparam elision #48144

merged 1 commit into from
Jan 6, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Jan 5, 2023

To catch a case that occurs in FuncPipelines.jl and was causing precision issues in #48066.

Comment on lines +830 to +836
length(applyTbody.parameters) == length(arg.parameters) || return nothing
for i = 1:length(applyTbody.parameters)
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar
return LiftedValue(argdef.args[3])
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it it may be clearer (and more extensible later) if written this way:

Suggested change
length(applyTbody.parameters) == length(arg.parameters) || return nothing
for i = 1:length(applyTbody.parameters)
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar
return LiftedValue(argdef.args[3])
end
end
# find a possible parameter number we are extracting when matching with `tvar`
i = findfirst(===(tvar), arg.parameters)
# determine if we can find the apply_type for that parameter
if applyTvar === applyTbody.parameters[i]
return LiftedValue(argdef.args[3])
end

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure whether we want to look at all the parameters or just the first one. We can probably look at the first one in applyTbody, but I think arg might potentially have multiple instances.

Copy link
Member

Choose a reason for hiding this comment

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

First is indeed probably the most common to extract

To catch a case that occurs in FuncPipelines.jl and was
causing precision issues in #48066.
Comment on lines +808 to +809
isa(arg, DataType) || return nothing
isType(arg) || return nothing
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isa(arg, DataType) || return nothing
isType(arg) || return nothing
isType(arg) || return nothing

isType(x) includes isa(x, DataType).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a follow up

@Keno Keno merged commit 1508425 into master Jan 6, 2023
@Keno Keno deleted the kf/morecsparams branch January 6, 2023 22:50
Keno added a commit that referenced this pull request Jan 6, 2023
Rolls up individual review comments from #48066, #48144, #48151.
Keno added a commit that referenced this pull request Jan 7, 2023
@vchuravy vchuravy added the backport 1.9 Change should be backported to release-1.9 label Feb 10, 2023
@vchuravy
Copy link
Member

@Keno is this backportable? #48612

@Keno
Copy link
Member Author

Keno commented Feb 10, 2023

I'd prefer not to. It's possible there further commits afterwards that are required

@dpinol
Copy link

dpinol commented Feb 14, 2023

@Keno Maybe in julia 1.9.1? #48612 shows that the impact is not negligible

@KristofferC
Copy link
Member

This has the backport label but @Keno says it shouldn't be backported. @vchuravy and @Keno, could you sort it out what it will be between yourself :P.

@vchuravy vchuravy removed the backport 1.9 Change should be backported to release-1.9 label Feb 14, 2023
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.

6 participants