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

add promote_symtype for _map and _mapreduce. #814

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

manuelbb-upb
Copy link
Contributor

This is meant to fix an inference problem.
MWE:

using Symbolics
@variables x[1:2]
norm_squared = sum( x .* 2)
simplified_norm_squared = simplify(norm_squared)

Now, norm_squared is a Num, but simplified_norm_squared is a Term{Any, Nothing}.

I believe the issue to be missing methods of promote_symtype for _map and _mapreduce.
Now, the interactions of symtype, typeof, promote_type and promote_symtype are still confusing to me, which is why the pull request is marked as draft.
Thoughts and comments would be appreciated :)

Anyway, these definitions appear to solve the issue and don't break the tests.
Also, we tackle the underlying problem, instead of what I suspected/suggested in this comment...

@ChrisRackauckas
Copy link
Member

This looks reasonable to me, but I wonder if this is still needed with Unityper's changes?

@manuelbb-upb
Copy link
Contributor Author

I have tried the MWE in a new environment with

[d1185830] SymbolicUtils v0.20.0-DEV `https://github.com/JuliaSymbolics/SymbolicUtils.jl.git#master`
[0c5d862f] Symbolics v4.13.0 `https://github.com/JuliaSymbolics/Symbolics.jl.git#d0a80ad`

where #d0a80ad is the latest commit in #549.
Here, simplified_norm_squared == Symbolics._mapreduce, which is even worse, I guess.

The original issue resulted from the outer most call to similarterm during simplification and a promote_symtype (or _promote_symtype?) therein.
From what I have gathered, the Unityper changes are meant to make calculations of terms more type stable w.r.t. their parameters, but I don't know if that extends to cases where promote_symtype is explicitly queried.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2023

Codecov Report

Merging #814 (47801aa) into master (a3fa921) will decrease coverage by 0.33%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #814      +/-   ##
==========================================
- Coverage   77.53%   77.20%   -0.34%     
==========================================
  Files          26       26              
  Lines        3201     3215      +14     
==========================================
  Hits         2482     2482              
- Misses        719      733      +14     
Impacted Files Coverage Δ
src/array-lib.jl 74.74% <0.00%> (-5.82%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    `_mapreduce`
2) introduce `_promote_symtype` instead, logic taken
    from the respective methods
3) adapt method definitions to avoid redundancy
4) some tests for the problem described in JuliaSymbolics#814
@manuelbb-upb
Copy link
Contributor Author

I have thought a bit about this. I cannot think of any situation where promote_symtype is needed for the (internal) functions _map and _mapreduce.
The initial issue described can be addressed by defining _promote_symtype instead.
We have access to all arguments here.
Thats why in f4d6121 I have taken the logic from the method definitions of _map and _mapreduce and put them into _promote_symtype.
This way, the return type should match and the issue is fixed, too.

)
end

function SymbolicUtils._promote_symtype(::typeof(_map), args)
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we should be extending this function... It looks like a very internal helper function. Why can't we do this with just promote_symtype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I attempted initially in 47801aa (btw sorry for the 3rd commit, did not know about git rebase)

I hope I remember correctly: The problem with promote_symtype for _map and _mapreduce is that promote_symtype is meant to have a signature promote_symtype(f, arg_symtypes...).
_promote_symtype receives the arguments instead of their types, i.e., _promote_symtype(f, args...).
In case of _map or _mapreduce we would like to recursively call promote_symtype for the mapped function and maybe a binary operator.
With promote_symtype I tried to obtain function instances by accessing the corresponding field of the type:

mapreduce(Base.Fix1(promote_symtype, F.instance), promote_type, eltype.(XS))
Works only for singleton types, but that should still catch almost all use-cases. Feels awkward nonetheless.
With _promote_symtype and the arguments available, we can use exactly the same logic as if actually calling _map and _mapreduce to get consistent type predictions (another problem with the promote_symtype approach).

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.

4 participants