-
Notifications
You must be signed in to change notification settings - Fork 21
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
Distribution Support & Mixed Sets #3115
Conversation
🦋 Changeset detectedLatest commit: 6b9e653 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
@@ -232,6 +233,10 @@ export class DiscreteShape implements PointSet<DiscreteShape> { | |||
(t) => t.shapeMap(XYShape.T.square).mean() | |||
); | |||
} | |||
|
|||
domain() { |
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.
todo: Change to "support"
case "lesserThan": | ||
return y < _threshold; | ||
case "equals": | ||
return Math.abs(y - _threshold) < Number.EPSILON; |
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.
There are probably subtle bugs here, or, at least, this boundary is arbitrary; Number.EPSILON
is not the smallest representable number.
Not sure if this is important.
return { | ||
points: sortBy(points, ([x]) => x), | ||
segments, | ||
}; |
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.
General thoughts on this function, without going into details too much:
- there are a few things here that could be optimized (e.g. multiple calls to
comparisonIsSatisfied
), but it's not very important - I'm kinda sad about it, because by now I did ~3 attempts to rewrite and clean up
XYShape
; it's hard to do incrementally and hard to do in a single day; so the more code this file has, the harder it'll be to do - but I guess it's worth it and not a blocker for the PR
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.
so the more code this file has, the harder it'll be to do
I agree, but at the same time, I don't expect it to be too bad to convert this to any other XYShape format, especially with tests + LLMs.
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.
Yea, I wasn't very focused on optimization. I don't expect we'll call this too often, and we can optimize if it ever seems like we'll want to, later.
support
functions to PointSet distributions, to get their support. This is a Mixed Set.extractSubsetThatSatisfiesThreshold
function to XYShape. This is kind of complicated. We only use part of it forsupport()
, but the rest was easy, and could be useful for later.Most of this is done in squiggle-lang and isn't exposed to the function registry. However, two functions are -
PointSet.support
and subtractingMixedSet
values.MixedSet
values are just stored and provided as Dicts.