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

Should we define our own Functions or use ones from Base? #12

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

Should we define our own Functions or use ones from Base? #12

zsunberg opened this issue Aug 9, 2022 · 3 comments

Comments

@zsunberg
Copy link
Member

zsunberg commented Aug 9, 2022

Currently, we use Base.eltype and Base.clamp in our interface. However this can be problematic since people tend to interpret these differently. For instance, eltype(1..2) returns Int. (and hence the broken test at

@test_broken eltype(tp) == Tuple{Float64, Float64}
)

Should we introduce CommonRLSpaces.element_type and CommonRLInterface.clamp to deal with these?

@findmyway
Copy link
Member

I personally prefer to use our own functions instead of reusing ones from Base. (Without exporting, we can still keep the same name)


Based on our discussions at #8 , a lesson I learned is, we can't use existing containers (like vectors, tuples...) and existing functions in Base together, otherwise conflicts like eltype here is unavoidable.

So we have two choices:

a. Reuse built-in containers but dispatch based on our own function (names)
b. Reuse functions in Base but dispatch based on our customized containers.

In the prior release, I adopted the second approach. The drawback is obvious that we have to always define a Space wrapper. Current code in the master branch seems to be shifting from #b to #a. But the interfaces like in, rand are still reused from Base. So maybe we should make a decision on it?

@zsunberg
Copy link
Member Author

zsunberg commented Aug 18, 2022

Hmm... yes, thanks for pointing out that this is a fundamental problem.

If we have to completely commit to one of those, I vote for "a" because that is most obvious for someone defining a simple environment/problem, e.g. actions = [-1, 0, 1].

However, ideally, we should not need to make this choice because everyone would agree on the abstract meaning of in, rand, eltype, and clamp. Indeed, it has worked so far on everything except for eltype(1..2), and this may have just been an oversight (I'm asking in JuliaMath/IntervalSets.jl#115).

Another note is that we cannot fully follow the principle of defining our interface in the CommonRLSpaces module because we can't re-define the iteration interface. Iteration is more fundamental than, say, clamp, but rand is also pretty fundamental. Where do we draw the line?

I need to think about this more.

@zsunberg
Copy link
Member Author

If there are other example packages to consider, that might be helpful.

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