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

Constraint revise #147

Merged
merged 11 commits into from
Jun 11, 2024
Merged

Constraint revise #147

merged 11 commits into from
Jun 11, 2024

Conversation

ocots
Copy link
Member

@ocots ocots commented Jun 9, 2024

Il faut finaliser ça.

@ocots ocots requested a review from jbcaillau June 9, 2024 18:41
@ocots
Copy link
Member Author

ocots commented Jun 9, 2024

There is a problem with the documentation. Maybe because of the basic example in functional form.

@ocots
Copy link
Member Author

ocots commented Jun 9, 2024

I think this does not work in this version (see here):

constraint!(ocp, :initial, [ -1, 0 ])           # initial condition
constraint!(ocp, :final,   [  0, 0 ])           # final condition

since there is no more

constraint!(
    ocp::OptimalControlModel,
    type::Symbol,
    val::Union{Real, AbstractVector{<:Real}}
)
constraint!(
    ocp::OptimalControlModel,
    type::Symbol,
    val::Union{Real, AbstractVector{<:Real}},
    label::Symbol
)

@ocots
Copy link
Member Author

ocots commented Jun 10, 2024

Do we want:

function constraint!(ocp::OptimalControlModel, 
    type::Symbol, 
    val::ctVector, 
    label::Symbol=__constraint_label())

    __constraint!(ocp, type, rg=nothing, f=nothing, lb=val, ub=val, label=label)
    nothing # to force to return nothing

end

or since we have:

function constraint!(ocp::OptimalControlModel, 
    type::Symbol, 
    lb::Union{ctVector,Nothing}, 
    ub::Union{ctVector,Nothing}, 
    label::Symbol=__constraint_label())

    __constraint!(ocp, type, rg=nothing, f=nothing, lb=lb, ub=ub, label=label)
    nothing # to force to return nothing

end

In the first case, we can write

constraint!(ocp, :initial, [ 1, 2, 1 ])

while if we do not have the first method then we should write

constraint!(ocp, :initial, [ 1, 2, 1 ], [ 1, 2, 1 ])

@ocots ocots mentioned this pull request Jun 10, 2024
@jbcaillau
Copy link
Member

@ocots @BaptisteCbl right, we should check and close it. plus the issue for time!.

@ocots
Copy link
Member Author

ocots commented Jun 11, 2024

I keep only the method constraint! with keywords.

@jbcaillau
Copy link
Member

I keep only the method constraint! with keywords.

@ocots OK 👍🏽 Note that there are indeed consistency checks in the general (__)constraint function (check the last case of the pattern matching below):

_ => throw(IncorrectArgument("Provided arguments are inconsistent"))

To me, there should only be some old functional syntax tests to update (some of them are already preventing the doc CI).

Then we must also take care of time! along the same line.

@ocots
Copy link
Member Author

ocots commented Jun 11, 2024

Up to now there is only one constraint! method.

There are indeed consistency checks but I have made more comprehensive the errors in the case of unauthorized calls:

(::RangeConstraint, ::Function) => throw(UnauthorizedCall("Providing a range (rg keyword) and a function (f keyword) is not authorized."))

Note that for the unitary tests, I have defined __constraint! to keep previous tests. See this file.

@ocots
Copy link
Member Author

ocots commented Jun 11, 2024

I think that constraint are revised (if the CI is ok) and we can make another branch for time! revision.

@ocots ocots mentioned this pull request Jun 11, 2024
Copy link
Member

Choose a reason for hiding this comment

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

@ocots is it really the only place where this is used?

Copy link
Member

Choose a reason for hiding this comment

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

@ocots I see some blue style here, well done 👍🏽 SNCF time 🙂

Copy link
Member

Choose a reason for hiding this comment

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

@ocots very nice tests update 👍🏽

Copy link
Member

Choose a reason for hiding this comment

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

@ocots well done 👍🏽👍🏽 rapid test update providing __constraint! that preserves the old functional style and is mapped (and so tests) the new constraint! with named args.

@jbcaillau jbcaillau merged commit 4418268 into main Jun 11, 2024
4 checks passed
@jbcaillau jbcaillau deleted the constraint_revise branch June 11, 2024 21:30
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.

3 participants