Skip to content
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

clamp() and clamp!() #115

Open
zsunberg opened this issue Aug 25, 2022 · 3 comments
Open

clamp() and clamp!() #115

zsunberg opened this issue Aug 25, 2022 · 3 comments

Comments

@zsunberg
Copy link
Contributor

An important operation for reinforcement learning contexts is to clamp an input to the nearest point within a set. Does anyone have objections to implementing Base.clamp and Base.clamp! for some sets in this package?

This would be similar to what is currently implemented in IntervalSets: clamp(3, 1..2) returns 2.

@daanhb
Copy link
Member

daanhb commented Aug 26, 2022

Hmm, this is different from rand. I don't know what clamp is supposed to do generically. For an interval, it returns the closest point in the set. For other domains, there are many ways to define a closest point, depending on some notion of distance, and DomainSets (in principle) has no distance metric. So, while it does not help you right now, ideally there is a package building on DomainSets which defines a distance metric - and then the closest point would makes sense.

@zsunberg
Copy link
Contributor Author

Hi @daanhb , I definitely understand hesitancy to add things to this package. Thanks for responding thoughtfully. However, I don't think adding clamp will add a terrible burden, so I hope you'll reconsider.

Hmm, this is different from rand. I don't know what clamp is supposed to do generically.

I would argue that rand has a similar underlying assumption, namely that it assumes a uniform distribution (hopefully this does not make you decide to remove rand 😂 ). We could sample from any distribution over the elements of the set, but we choose by default to sample uniformly, and the choice of what "uniform" means assumes some kind of volume measure anyways - there could be other choices for that.

DomainSets (in principle) has no distance metric.

The concept of a ball fundamentally assumes a distance metric, so I would argue that DomainSets assumes a default metric of norm, i.e. the Euclidean Metric.

I am proposing that we extend the existing decision for Balls that when we need a metric, we default to Euclidean. This does not prohibit someone else from creating a more fully-featured package that allows for multiple distance metrics.

@daanhb
Copy link
Member

daanhb commented Sep 2, 2022

That's a fair point. I guess a good way to implement a distance metric would be to implement norm, so any implementation in terms of norm (such as Ball and friends) would carry over. The norm function has a second argument to specify which norm (clamp doesn't), or alternatively the metric space could be encoded into type T.

In fact we already have the distance_to function, which seems a worse offender than clamp. Perhaps that function should allow an extra argument to pass to norm. And perhaps we should have more general balls that allow to specify a different norm.

Anyway, if clamp is useful to you, go ahead.

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

No branches or pull requests

2 participants