edwardkort / WWIDesigner

Wood Wind Instrument Designer
43 stars 8 forks source link

Optimizers working on fixed bore profiles can move bore points #99

Open edwardkort opened 3 years ago

edwardkort commented 3 years ago

In the NAF study, there are optimizers that work on fixed bore profiles: Grouped-hole position & size, and Hole size and position. However, because they also change bore length, there is the potential to modify existing non-terminal fixed bore points. Two scenarios are problematic:

  1. the bore-length constraints are too broad, encompassing bore points outside of the final solution
  2. the final solution encroaches upon an existing non-terminal bore point.

The current implementation, when a bore point is encroached, moves the bore point a small amount. In the course of an optimization, this bore point may be moved quite a bit - resulting in a very different bore profile.

We commented, in the relevant source code, that this might be a problem - but didn't provide a solution. I propose the following:

  1. Add a length-optimization type (there are already several) for use in fixed-bore-profile optimizers
  2. Check whether a bore point is encroached (code is already there in order to incrementally move the bore point)
  3. Throw the appropriate exception (to be written) if a bore point is encroached
  4. Display an informational exception method indicating that the user should either tighten up the length constraint (scenario 1) or delete the bottom bore point in the input instrument file (scenario 2).

I don't see a way to do the above programmatically, since the two scenarios are indistinguishable during the optimization iterations. But input is welcome.

bhp1 commented 3 years ago

Would it be feasible to set the minimum bore length constraint to prevent encroaching on a bore point? (I don't think so, because this could be used in a merged objective function that alters bore point positions.)

Much of the problem with moving bore points is that setGeometryPoint() doesn't move them back where they were if the new bore length is large enough. Would it be feasible to save the original bore point positions, and put them in their original places unless they have to be shifted because the bore length encroaches on them? (There could be a conflict if used in a merged objective function, but I think it is feasible, perhaps with a new BoreLengthAdjustmentType.)

Throwing an exception when the bore length encroaches on a bore point would interrupt the entire optimization. I think this would turn out to be unwelcome.

BoreLengthAdjustmentType PRESERVE_BELL might help reduce the disruption from moving bore points, perhaps with a modified implementation of findBell() that uses bore point names. The profile at the end of the bore remains the same; the bore length is varied by changing the length of a higher bore segment rather than the lowest.

edwardkort commented 3 years ago

Burton,

Thanks for looking at this. Addressing your points:

For the broad constraint scenario, it would be possible to put a validation check in the StudyView, much like we have for mismatched number of holes between the instrument and tuning; it simply checks whether the lower length bound is less than the second bore-point position. I find the hole mismatch warning annoying, so don't want to add yet another one.

For now, I propose just throwing the appropriate informative exception. If that is too onerous, and I seem to be the only user to have found this bug, we can further investigate more automated solutions.

edwardkort commented 3 years ago

For the case of too broad bore-length constraints, intermediate bore points are only moved when invoking multi-start optimization. Since the NAF "fixed bore" optimizers never (in my experience) benefit from multi-start, this fix turns off multi-start optimization for these optimizers (and notifies the user in a Console message.

For the case of optimization shortening the bore above an intermediate bore point, an exception is thrown identifying the suggested user fix. This fix is easy to implement programmatically, but I have worries that all the scenarios have not been identified. So for now, an exception.

In adding the new fixed-bore optimization constraint, the following changes and refactoring were made:

All tests pass, no non-NAF functionality should have been changed. However, without complete code coverage in our tests, I could have missed something.

Burton, please perform a happy-path test of the whistle, reed, and flute studies.

edwardkort commented 3 years ago

Fix will be in v 2.6.0.

edwardkort commented 3 years ago

I may need to reopen this issue; i.e., modify the fix that was implemented in v 2.6.0. I made the statement above, and implemented in the fix: "Since the NAF "fixed bore" optimizers never (in my experience) benefit from multi-start, this fix turns off multi-start optimization for these optimizers (and notifies the user in a Console message)."

Recently I was designing some very small (high pitched) NAFs with a fundamental of E5. It appears that such instruments, with a much stronger interaction with the hole size/position constraints, would benefit from multi-start optimizations. If/when I confirm this, I will then modify the logic of this fix to throw an exception rather than revert to single-start optimization in the fixed-bore scenarios.