Skip to content

Add canonical #3008

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.

Sign up for GitHub

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

Draft
blegat wants to merge 1 commit into
base: master
Choose a base branch
from
+26 −0
Draft

Add canonical #3008

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/Utilities/functions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,32 @@ function canonical(f::MOI.AbstractFunction)
return g
end

# Workaround: both `is_canonical` and `canonicalize!` would be slow otherwise
canonical(f::MOI.ScalarNonlinearFunction) = f

@odow odow Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ask Claude why they would add this method when there is the one below 😆

@blegat blegat Jun 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did it pre-Claude actually 😅 this line is just a temporary hack to isolate things in profiling


# This is maybe the right fix, but commented out to be sure to isolate
# JuMP-side of profiling in https://github.com/jump-dev/JuMP.jl/pull/4032
# We still need to figure out what's the right fix here anyway
#function canonical(f::MOI.ScalarNonlinearFunction)
# cache = Dict{MOI.AbstractScalarFunction,MOI.AbstractScalarFunction}()

@blegat blegat Jun 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The goal of this was to write a version with this cache but I first want to check that this is really necessary

# # Don't use recursion here. This gets called for all scalar nonlinear
# # constraints.
# stack = Any[arg for arg in f.args]
# while !isempty(stack)
# arg = pop!(stack)
# if arg isa MOI.ScalarNonlinearFunction
# for a in arg.args
# push!(stack, a)
# end
# else
# if !is_canonical(arg)
# return false
# end
# end
# end
# return true
#end

@odow odow Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete this code? It isn't correct regardless, because the function is canonical but it returns a ::Bool instead of the canonicalised function.

@odow odow Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not change the is_canonical function? With this change it can be that is_canonical(canonical(f)) == false

@odow odow Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the underlying problem? That we walk the expression graph?

@blegat blegat Jun 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wrote this in July 2025, I don't remember much, I will need to look at it again but let's first focus on merging jump-dev/JuMP.jl#4032, then we'll look at the MOI level


canonicalize!(f::Union{MOI.VectorOfVariables,MOI.VariableIndex}) = f

"""
Expand Down
Loading