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

Version 1.0 #14

Closed
7 tasks done
moorepants opened this issue Jan 27, 2017 · 12 comments
Closed
7 tasks done

Version 1.0 #14

moorepants opened this issue Jan 27, 2017 · 12 comments

Comments

@moorepants
Copy link
Collaborator

moorepants commented Jan 27, 2017

There are some warts that would be nice to address which require backwards incompatibility, maybe warranting a 1.0 release.

  • Make the main class Problem instead of problem.
  • Use PEP8 standards (particularly method names to the problem class).
  • Rename the main module from ipopt to cyipopt
  • Move src into a sub-directory of the main module.
  • Move test to examples.
  • Create some unit tests and run them on CI.
  • Make scipy optional.

Thoughts?

@matthias-k
Copy link
Collaborator

@moorepants All of this sounds very reasonable to me. If you would like to, feel free to go ahead on this, I probably won't have to much time over the next days.

@brocksam
Copy link
Collaborator

brocksam commented Jul 2, 2020

I've begun to put together a branch in my fork that will be a new major version proposal[*]. It covers the suggestions that @moorepants outlined above (including issue #28) as well as a bit of general tidying up. Would be great to get a 👍 or 👎 from other contributors as to whether they agree it's along the right lines before I do more work on it (adding more tests, updating the docs etc.).

Also, thoughts on discontinuing support for Python 2.7 in a new major version? Of the five dependent packages shown in the dependency tree, all are Python 3.6+ except oats and opty, oats looks like it's strictly Python 2 while opty still supporting Python 2.7 alongside Python 3.6+. In my opinion dropping Python 2.7 support would allow significant code cleanup without the hassle of ensuring backwards compatibility. Happy to hear arguments otherwise though.

[*] Note that the Travis build is currently failing with Python 2.7 only because I've used f-strings. Can easily remedy this if there is disagreement with my above proposal.

@moorepants
Copy link
Collaborator Author

Great!

Please open a pull request from your branch so we can make comments.

I'm fine dropping Python 2.7 in the next release.

@apommel
Copy link
Contributor

apommel commented Jul 2, 2020

All of this seems reasonable, including dropping Python 2 as older release will still support it. However it will probably be easier to review if the branches were separated by feature.
Also, in the current state, it conflicts with #63 so we would probably want to merge that before so you can solve the conflicts.

@moorepants
Copy link
Collaborator Author

However it will probably be easier to review if the branches were separated by feature.

Agreed. @brocksam Can you submit PRs feature by feature?

@moorepants
Copy link
Collaborator Author

Rename the main module from ipopt to cyipopt

We need to think about how to do this in a deprecated way. It may or may not be possible.

@brocksam
Copy link
Collaborator

brocksam commented Jul 2, 2020

Yes, I will endeavour to submit PRs feature-by-feature, however the first one may need to be rather big as I've had to change a fair bit of stuff in parallel to keep the package working. Can we create a new branch (e.g. version-1) on this base repo that I can submit the PRs from my fork to? Might make sense to do it this way if I will be submitting a number of PRs before the refactor is "done". Then we can merge it in to master in one go.

Rename the main module from ipopt to cyipopt

We need to think about how to do this in a deprecated way. It may or may not be possible.

I think I've managed to do this, assuming I've correctly understood what you meant. When the user tries to import using pythonimport ipopt it issues a FutureWarning on the console and then hands-off to import cyipopt. There's a passing test for it in tests/unit/test_deprecations.py called test_ipopt_import_deprecation for this functionality.

@brocksam
Copy link
Collaborator

brocksam commented Jul 2, 2020

All of this seems reasonable, including dropping Python 2 as older release will still support it. However it will probably be easier to review if the branches were separated by feature.
Also, in the current state, it conflicts with #63 so we would probably want to merge that before so you can solve the conflicts.

I'll submit first PR once #63 is merged in this base repo and I've fixed conflicts.

@moorepants
Copy link
Collaborator Author

Ok, we'll try to get #63 in soon.

@brocksam
Copy link
Collaborator

Any news on progress with #63?

Can we create a new branch (e.g. version-1) on this base repo that I can submit the PRs from my fork to? Might make sense to do it this way if I will be submitting a number of PRs before the refactor is "done". Then we can merge it in to master in one go.

And thoughts on this request?

@brocksam
Copy link
Collaborator

Sorry to sound like a broken record, but any news yet on #63 going in?

@moorepants
Copy link
Collaborator Author

This is all taken care of. We can open separate issues for any other 1.0 changes that we want before releasing.

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

No branches or pull requests

4 participants