Skip to content

Simplified boundary enforcement#283

Merged
gpampara merged 21 commits intociren:masterfrom
gpampara:simplified-enforcement
May 18, 2018
Merged

Simplified boundary enforcement#283
gpampara merged 21 commits intociren:masterfrom
gpampara:simplified-enforcement

Conversation

@gpampara
Copy link
Copy Markdown
Member

These changes build on the contribution by @KyleErwin, but instead attempt to simplify
the usage/enforcement of boundary constraints by just letting the strategies be functions passed
to an enforce function.

The usage within Step etc is not part of these changes, albeit trivial. It shouldn't be
needed to provide these functions, but if needed they will be added in a separate PR.

Copy link
Copy Markdown
Contributor

@KyleErwin KyleErwin left a comment

Choose a reason for hiding this comment

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

This looks really great. Especially seeing how the code as evolved since the first iteration of bchms. The enforce and eforceTo methods are also really clever. Will definitely help in the long run.

@gpampara gpampara force-pushed the simplified-enforcement branch from 65722f7 to 2d4c323 Compare May 13, 2018 16:45
def clamp[A](implicit N: spire.math.Numeric[A]) =
(a: A, b: Interval[Double]) =>
if (N.toDouble(a) > b.lowerValue) N.fromDouble(b.upperValue)
else if (N.toDouble(a) < b.upperValue) N.fromDouble(b.lowerValue)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be if a > upperValue and a < lowerValue?

if (b.contains(N.toDouble(a))) a else N.fromAlgebraic((b.upperValue + b.lowerValue) / 2.0)

def reflect[A](implicit N: spire.math.Numeric[A]) =
(a: A, b: Interval[Double]) => if (b.contains(N.toDouble(a))) a else -a
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if b does not contain -a?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right. I just did a plain translation of the original code. I'll check what we did in the java version - I know it was right there - and port the logic over.

@gpampara
Copy link
Copy Markdown
Member Author

@filinep Please can you comment on the new changes. I think that they are more sane now :)

val z = N.toDouble(a)

if (z < b.lowerValue) N.fromDouble(b.lowerValue - (z - b.lowerValue))
else if (z > b.upperValue) N.fromDouble(b.upperValue - (z - b.upperValue))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about cases where the reflection makes it go passed the other bound? e.g. b=(5,6) a=3 the reflected value is 7

Copy link
Copy Markdown
Member Author

@gpampara gpampara May 16, 2018

Choose a reason for hiding this comment

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

Looking at some more literature, the paper I originally was basing these equations off of, was actually wrong. The equation has been adjusted and some tests have been added to ensure that they are correctly within the bounds.

val z = N.toDouble(a)

if (z < b.lowerValue) N.fromDouble(b.lowerValue + (z - b.lowerValue))
else if (z > b.upperValue) N.fromDouble(b.upperValue + (z - b.upperValue))
Copy link
Copy Markdown
Member

@filinep filinep May 15, 2018

Choose a reason for hiding this comment

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

If I understand this correctly, shouldn't the logic be
if (z < lowerValue) upperValue - (z - lowerValue) else if (z > upperValue) lowerValue - (z - upperValue) else z
You have to come from the other side instead of the same side it has gone over.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yip, exactly right - it does an environment "wrap", kinda like the old platform games.

implicit def fooGen = Arbitrary {
for {
i <- intervalGen
range = math.abs(i.upperValue - i.lowerValue) * 0.4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using 0.4 never generates the breaking example I gave above if b=(5,6) and a=3, this case still breaks the equation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it needs to be at least 2 times the range

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a fundamental issue with this boundary mechanism. Do we just keep reflecting until the value is in range? Does that even make sense? I've not seen such an adjustment in anything that I've read so far.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hehe, I guess you can keep reflecting, it's similar to what the periodic one does with the mod.

Copy link
Copy Markdown
Member Author

@gpampara gpampara May 16, 2018

Choose a reason for hiding this comment

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

Yeah, that's the only thing that could possibly work... seems a little strange though. I guess it can't be too bad then, because the point of the schemes to to bring something back into the valid search space. They all make the assumption that the initial starting point is within the bounds.

filinep
filinep previously approved these changes May 17, 2018

val result =
if (z < b.lowerValue) N.fromDouble(b.lowerValue + (b.lowerValue - z) % range)
else if (z > b.upperValue) N.fromDouble(b.upperValue - (z - b.upperValue) % range)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I pictured reflection to bounce between the upper and lower bound until it's valid. To do that you'd need to know how many bounces to do to know whether to come from the lower or upper side. But since we don't know how reflection is supposed to be dealt with if it's still out of bounds I'll approve it. I'll review again if you decide to change it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yip, indeed. During the testing, there was a point generated with and exponent of E208 but the interval was far far smaller. it would have taken hours to reduce to the interval.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True, but wouldn't it be possible to calculate how many bounces with a mod, instead of using a recursive function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it should be - as long as the function is decreasing to ensure a solution.

filinep
filinep previously approved these changes May 18, 2018
Copy link
Copy Markdown
Member

@filinep filinep left a comment

Choose a reason for hiding this comment

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

I have a feeling reflect can be done with one equation but this looks good to me

gpampara added 4 commits May 18, 2018 08:43
The reflect strategy behaves a bit strangely. It will work
correctly until the value is: > 2*range of domain

If the value is larger, the resulting value will _still_ be
outside the feasible range of the problem space. This change will
essentially ignore and then shrink the value so that it is within
the 2*range before doing the reflect back into the feasible space.
It is not clear if this is _valid_, however, in the normal usage case
this shrinking behaviour would never be done.
@gpampara gpampara merged commit 59c6175 into ciren:master May 18, 2018
@gpampara gpampara deleted the simplified-enforcement branch May 18, 2018 07:02
gpampara added a commit that referenced this pull request May 21, 2018
* Added a few bchms

* Generic methods

* Formating

* flatten  for tuples

* Moved to a nchm object in Entity

* Removed tuple flatten

* scalafmt

* No partial functions

* if statements

* Simplify boundary constraints to be plain functions

* Cleanup (more) of functions

* Some more cleanup

* Simplify bondary enforcement to a single function

* Adjust structures to make inference by compiler better

This new scheme now allows for a context for a given boundary handling
mechanism. Some of the strategies require that the new value is
randomly initialized between the domain bounds.

* Correct deployment branch: series/2.0.x -> master

* Add tests for boundary enforcement and correct toroidal scheme

* Adjust reflect to consider the offending portion only

The reflect strategy behaves a bit strangely. It will work
correctly until the value is: > 2*range of domain

If the value is larger, the resulting value will _still_ be
outside the feasible range of the problem space. This change will
essentially ignore and then shrink the value so that it is within
the 2*range before doing the reflect back into the feasible space.
It is not clear if this is _valid_, however, in the normal usage case
this shrinking behaviour would never be done.

* Correct formatting

* Continuous reflecting until within range

* Scalafmt changes

Co-authored-by: Kyle Erwin <kyle.erwin24@gmail.com>
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.

3 participants