Add bounded and inequality operand definition#53
Add bounded and inequality operand definition#53HarrisonKramer merged 11 commits intoHarrisonKramer:masterfrom
Conversation
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Thanks for the addition! This will be very useful. Will review shortly. Just granted you access. Please kindly continue to use PRs for all changes, unless something really minor. |
HarrisonKramer
left a comment
HarrisonKramer
left a comment
There was a problem hiding this comment.
This is an impressive addition and extremely useful.
Only one critical remark:
- I propose to use only
min_valandmax_valinoperand.pyto define bounds. Theboundsargument appears to be redundant in this case, but please correct me if I miss some logic. I think this would simplify the logic of that dataclass and the downstream code.
I will definitely want to add quite a few tests to cover the new functionalities, as well as many new examples for the docs, but those are not blocking. Everything looks logical and existing tests pass. I can add more docs/tests later on.
Thanks again! let me know if you want a hand with any changes, or have other remarks.
Kramer
There was a problem hiding this comment.
These details are not critical at all, but consider:
- run the optimization with a high
tolto get a better final result e.g.,res = optimizer.optimize(tol=1e-9) - no big deal, but the image height operands are still pretty far off after optimization. I'm curious if there's any fix for that e.g., increasing weights or changing starting point. Again, not critical and no real need to change.
| target (float): The target value for the operand. | ||
| operand_type (str): The type of the operand. | ||
| target (float): The target value of the operand. | ||
| bounds (list): The operand should stay between the bounds (bounded operand). |
There was a problem hiding this comment.
- what do you think about using
min_valandmax_valinstead ofmore_thanandless_than? These would then be the same as those used in the variables (alsomin_valandmax_val) and it is more intuitive (at least for me). - Do we need both
boundsand themin_val/max_val? Perhaps I miss some of the logic, but would it be enough only to usemin_valandmax_val? They define bounds too, after all. Unless this breaks something I'm missing, I strongly advise making this change and propagating it through the rest of this file. (and other files).
| self.operands = [] | ||
|
|
||
| def add(self, operand_type, target, weight=1, input_data={}): | ||
| def add(self, operand_type=None, target=None, bounds=None, more_than=None, less_than=None, weight=1, input_data={}): |
There was a problem hiding this comment.
if you agree with the change to operand.py, then you can update these arguments
| for op in self.operands], | ||
| 'Target': [op.target for op in self.operands], | ||
| 'Target': [f'{op.target:+.3f}' if op.target is not None else '' for op in self.operands], | ||
| 'Bounds': [op.bounds if op.bounds else '' for op in self.operands], |
There was a problem hiding this comment.
if you agree with changing bounds argument in operand.py, then consider using 'Min. Bound' and 'Max. Bound' as key names, which matches those used for variables.
|
Thanks for the access! You're right I implemented the changes, the class is simpler now |
HarrisonKramer
left a comment
HarrisonKramer
left a comment
There was a problem hiding this comment.
Looks solid!
I think you can simplify the logic of Operand.delta_ineq slightly (see comment), then should be good to merge.
| then this operand simply is zero. | ||
| """ | ||
|
|
||
| # One of the two |
There was a problem hiding this comment.
I don't think you need 3 conditions. Lines 198-208 can be simplified into the following (you can confirm):
lower_penalty = max(0, self.min_val - self.value) if self.min_val is not None else 0
upper_penalty = max(0, self.value - self.max_val) if self.max_val is not None else 0
return lower_penalty + upper_penaltyThere was a problem hiding this comment.
It seems to work thanks!
HarrisonKramer
left a comment
HarrisonKramer
left a comment
There was a problem hiding this comment.
Looks good! We can merge.
2 participants
This PR introduces an implementation of a very usefull feature : bounded and inequality operands.
Up until now, an operand could only be defined with a
targetargument.problem.add_operand()now accepts either atarget,boundsor inequality boundsmore_thanandless_than. Checks are done to ensure that conflicting parameters cannot be set at the same time. Specifying bothmore_thanandless_thanhas the same effect as specifyingbounds.I also fixed tests that were not passing (but I didn't add any for this new feature), all tests should now pass.
Please review carefully before merging, as this is a critical optimization feature.
Nb: Would you be so kind as to give me the permissions to Optiland's repo ? I could push directly without going through a fork and contributing to other PRs would be easier. Thanks!