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

[RTR] SQP method interface #556

Merged
merged 9 commits into from
Oct 26, 2022
Merged

Conversation

EveCharbie
Copy link
Collaborator

@EveCharbie EveCharbie commented Oct 20, 2022

All Submissions:

  • Have you followed the guidelines in our Contributing document [docs/contribution.md]?
  • Have you checked to ensure there aren't other open [Pull Requests] for the same update/change?
  • [] Have you opened/linked the issue related to your pull request?
  • Have you used the tag [WIP] for on-going changes, and removed it when the pull request was ready?
  • When ready to merge, have you sent a comment pinging @pariterre in it?

New Feature Submissions:

  1. Does your submission pass the tests (if not please explain why this is intended)?
  2. Did you write a proper documentation (docstrings and ReadMe)
  3. Have you linted your code locally prior to submission (using the command: black . -l120 --exclude "external/*")?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new examples for your core changes, as applicable?
  • Have you written new tests for your core changes, as applicable?

This change is Reviewable

Copy link
Member

@pariterre pariterre left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @EveCharbie)


bioptim/interfaces/ipopt_interface.py line 17 at r1 (raw file):

class IpoptInterface(SolverInterface):

Copy-paste all this file and move common function if it makes sense


bioptim/interfaces/solver_options.py line 25 at r1 (raw file):

        # @abstractmethod
        # def set_convergence_tolerance(self, tol: float):
        #     """

do not comment, raise when called

Code quote:

        # @abstractmethod
        # def set_convergence_tolerance(self, tol: float):
        #     """
        #     This function set the convergence tolerance
        #
        #     Parameters
        #     ----------
        #     tol: float
        #         Global converge tolerance value
        #     """

bioptim/interfaces/solver_options.py line 35 at r1 (raw file):

        # @abstractmethod
        # def set_constraint_tolerance(self, tol: float):

do not comment, raise when called

Code quote:

        # @abstractmethod
        # def set_constraint_tolerance(self, tol: float):
        #     """
        #     This function set the constraint tolerance.
        #
        #     Parameters
        #     ----------
        #     tol: float
        #         Global constraint tolerance value
        #     """

bioptim/interfaces/solver_options.py line 68 at r1 (raw file):

        # @abstractmethod
        # def set_print_level(self, num: int):

do not comment, raise when called

Code quote:

        # @abstractmethod
        # def set_print_level(self, num: int):
        #     """
        #     This function set Output verbosity level.
        #
        #     Parameters
        #     ----------
        #     num: int
        #         print_level
        #     """

bioptim/interfaces/solver_options.py line 387 at r1 (raw file):

    @dataclass
    class SQP_METHOD(Generic):
        type: SolverType = SolverType.SQP_METHOD

type showld not be necessary


bioptim/optimization/optimal_control_program.py line 931 at r1 (raw file):

            elif solver.type == SolverType.SQP_METHOD:
                from ..interfaces.ipopt_interface import IpoptInterface

SqpInterface

Copy link
Collaborator Author

@EveCharbie EveCharbie left a comment

Choose a reason for hiding this comment

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

did not test it since I tried to reach convergence without success (tried many different number of nodes and even giving the solution from IPOPT as initial guess).

Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @pariterre)


bioptim/interfaces/solver_options.py line 25 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

do not comment, raise when called

Done.


bioptim/interfaces/solver_options.py line 35 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

do not comment, raise when called

Done.


bioptim/interfaces/solver_options.py line 68 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

do not comment, raise when called

Done.


bioptim/interfaces/solver_options.py line 387 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

type showld not be necessary

Done.


bioptim/optimization/optimal_control_program.py line 931 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

SqpInterface

Done.

@pariterre
Copy link
Member

@EveCharbie hum.. okay.. if an example as simple as the pendulum can't converge, I must admit I am questionning the usefulness of the SQP method haha

@EveCharbie
Copy link
Collaborator Author

@EveCharbie hum.. okay.. if an example as simple as the pendulum can't converge, I must admit I am questionning the usefulness of the SQP method haha

I totally agree, but I had to implement it to test. So now that it is implemented, we have to decide if we keep it. Form aceglia's experience with QP solver, we would have to fine-tune extensively the parameters of the solver in order to reach convergence, but it might be relevant to proceed this way in some specific cases.

@pariterre
Copy link
Member

@EveCharbie
Ok, anyway it makes sense to keep it, and I honestly don't really mind if the test converges or not, as long as it does this consistently (as the goal is to detect if the behavior changes). I feel sorry though that the example itself that we provide doesn't converge.. We can keep this for the future anyway!

Copy link
Collaborator Author

@EveCharbie EveCharbie left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @pariterre)


bioptim/interfaces/ipopt_interface.py line 17 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Copy-paste all this file and move common function if it makes sense

Not sure I did it right but ok.

@EveCharbie EveCharbie changed the title [WIP] SQP method interface [RTR] SQP method interface Oct 25, 2022
Copy link
Member

@pariterre pariterre left a comment

Choose a reason for hiding this comment

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

@EveCharbie RTM?
:lgtm:

Reviewed 3 of 5 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)

@EveCharbie
Copy link
Collaborator Author

Yes RTM :)

@pariterre pariterre merged commit 1412971 into pyomeca:master Oct 26, 2022
@EveCharbie EveCharbie deleted the add_SQPmethod branch May 30, 2023 07:12
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.

None yet

2 participants