-
Notifications
You must be signed in to change notification settings - Fork 97
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
Successive constraints method #1989
base: main
Are you sure you want to change the base?
Conversation
Seems like I should complain more often ... Maybe we can discuss things offline next week? |
Yes of course, let's discuss this next week. |
55a155b
to
61d1f23
Compare
@sdrave Do you think you will have time to discuss and look into this before the release? |
|
61d1f23
to
1c0bcd7
Compare
15ac296
to
65ab174
Compare
It would be great if you could take a look at this @sdrave when you find time. |
The pipelines are failing due to a very old scipy version (1.5.4) where the 'highs' method (which is now the default) was not available as solver of a linear program in scipy. 'simplex' would be a method that is supported in probably all versions but is considered legacy in current versions. So maybe also not the best option to use that one as default. How about pinning scipy to >=1.7.0? |
Debian oldstable has 1.6.0. I think that should be enough as well? Any reasons to require 1.7? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @HenKlei, I think the code generally looks nice.
My biggest concern is that you have to solve the same eigenproblems ('SM') for both the upper and lower bound functional (and the upper bound functional does not allow passing the solutions via __init__
). I wonder whether it makes more sense to refactor the code into an algorithms.scm
module, with a function which instantiates both functionals. That would be somewhat less elegant, but would avoid significant offline costs.
Further remarks:
- Have you considered also implementing the greedy algorithm from the paper? Should be fairly straightforward.
- I have never looked into this personally, but how does the situation look like for inf-sup stable problems? Can we easily support this as well?
- As already discussed, it would be good to extend the demo/add another demo to validate the performance of the method (only looking at the coercivity constants).
You're right, 1.6.0 would be sufficient. |
Co-authored-by: Stephan Rave <stephanrave@uni-muenster.de>
Is the scm module in algorithms what you anticipated @sdrave? I will look into your further remarks in the next days. |
Since @sdrave was complaining about the missing successive constraints method for a few times now, I started implementing something in that direction. However, this is far from being finished at the moment.