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

Rename variablewise functions #60

Closed
mlubin opened this issue Jul 18, 2017 · 11 comments
Closed

Rename variablewise functions #60

mlubin opened this issue Jul 18, 2017 · 11 comments

Comments

@mlubin
Copy link
Member

mlubin commented Jul 18, 2017

The name "variablewise function" is a holdover from variablewise constraint, which made a bit more sense.
To replace ScalarVariablewiseFunction and VectorVariablewiseFunction, what about ScalarComponentProjection and VectorComponentProjection?

Let the bikeshedding begin. @blegat @joehuchette @chriscoey @odow @joaquimg @IssamT

Thanks to @edljk for pointing this out.

@IssamT
Copy link
Contributor

IssamT commented Jul 18, 2017

or ScalarIdentityFunction and VectorIdentityFunction ?

@mlubin
Copy link
Member Author

mlubin commented Jul 18, 2017

The issue is that these are not identity functions by any usual definition of identity, except when all variables appear in the map. They are coordinate-wise projections by any usual definition.

@blegat
Copy link
Member

blegat commented Jul 18, 2017

Maybe ScalarProjectionFunction and VectorProjectionFunction to keep the Function suffix ?

@mlubin
Copy link
Member Author

mlubin commented Jul 18, 2017

A projection is a function, so projection function is a bit redundant. Not all sets end with Set.

@mlubin
Copy link
Member Author

mlubin commented Jul 18, 2017

We could say ScalarProjection and VectorProjection to keep it shorter.

@mlubin
Copy link
Member Author

mlubin commented Jul 18, 2017

Although in the future I can imagine someone wanting to add other kinds of projections.

@odow
Copy link
Member

odow commented Jul 18, 2017

What's wrong with Variablewise?

I'd almost prefer

ScalarFunction(Variable, args... )
ScalarFunction(Affine, args... )
ScalarFunction(Quadratic, args... )

addconstraint!(m, ScalarFunction(Affine, x, [1.0,1.0], 0.0), LessThan(5.0))

or

VectorFunction{Variable}(args... )
VectorFunction{Affine}(args... )
VectorFunction{Quadratic}(args... )

or just ScalarFunction and VectorFunction and the type is inferred from the number of arguments.

@mlubin
Copy link
Member Author

mlubin commented Jul 18, 2017

What's wrong with Variablewise?

Affine and quadratic functions are well-defined and widely used concepts. Variablewise is not. Why not just use the mathematically correct and clear name for the function?

It could be useful to formalize the scalar/vector concept (as @blegat attempted to do in #44) since it seems like we've converged to definitions of sets that are exclusively scalar or vector sets. What are the Variable, Affine, and Quadratic objects though? Wouldn't it be confusing to have a type called Variable which is just a placeholder for dispatch and not an actual variable?

or just ScalarFunction and VectorFunction and the type is inferred from the number of arguments.

That's not extensible, so I'd downvote that.

@mlubin
Copy link
Member Author

mlubin commented Jul 19, 2017

Another idea: SingleVariable and VectorOfVariables.
These seem more self-explanatory than ScalarVariablewiseFunction and VectorVariablewiseFunction. The description "single" is meant to evoke the idea that you're extracting one variable out of all of the variables in the model:

addconstraint!(m, SingleVariable(x[1]), LessThan(10.0))
addconstraint!(m, VectorOfVariables(x[1:5]), AllDifferent(5)) # thinking ahead...

It wouldn't be too crazy to have a fallback like:

addconstraint!(m, v::VariableReference, s) = addconstraint!(m, SingleVariable(v), s)
addconstraint!(m, v::Vector{VariableReference}, s) = addconstraint!(m, VectorOfVariables(v), s)

but that's a mostly separate discussion.

@joaquimg
Copy link
Member

This is my favorite among all the other suggestions.
And I liked the fallback

@blegat
Copy link
Member

blegat commented Jul 19, 2017

I am ok with either ScalarProjection/VectorProjection or SingleVariable/VectorOfVariable and I upvote the callback :)

odow added a commit that referenced this issue Sep 23, 2019
* Add options to MOF

* Improve code coverage

* Improve test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants