-
Notifications
You must be signed in to change notification settings - Fork 190
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
UnitWeights #358
Comments
I'm in favor in principle, but why would we need to actually store a vector of weights if we know they are unit weights? I would have thought we should use an implementation similar to #135. |
I would be fine with
Then just specialize the methods for efficiency. |
If we add this, it should also implement |
Something like?
|
Mostly, though that's a bit more complex, see #135. |
FillArrays provides the |
I have redesigned |
Sure, please file a PR! Though looking at your implementation it appears to allow any value, while "unit" evokes weights equal to 1 to me, doesn't it? |
True. As I understand it, the current inner constructor should prevent the user from creating weights with a different value, but the user could later modify it. The following alternative implementation is safer, albeit marginally less efficient:
We need then change references to
becomes
Does it seem better? |
I would have to test it, but |
|
I believe it conforms to Maybe something like
|
My bad. I didn't know that ranges were a subtype of arrays. Wouldn't it be simpler to keep the current definition and have |
Probably the best solution, but might require to break the Disclosure, I did add an opinionated |
What's the purpose of |
I would favor relaxing the |
It might also be better to have a |
What would be the advantage over returning the |
The constructor for |
I will make a pull request with the current implementation. We can then review it as necessary. I think however that broader changes to the weights infrastructure belong to a separate pull request. |
What I propose is that |
If you want open the PR for now,
|
I thought we wanted to avoid holding the weight vector. To quote @nalimilan above,
|
For making it a subtype of Relaxing that requirement was the discussion I mentioned above, but since we can discuss that at a later stage for including You could still need to materialize the input for wrapping it in a
That would work after The current API requires an |
Sorry, I don't follow.
Why? The current implementation doesn't hold the weight vector in memory, but it works fine: you can index it, you can iterate on it, you have an appropriate
Why would you need to materialize the input? |
The API doesn't require fields, it requires methods. Also, |
What I am trying to say is that all other weights have a constructor |
Good point. But the proposed implementation already takes a number instead of an array. |
Aye. That is why it currently calls |
No, I suggested |
|
I think we can just remove the |
Aye. That was my suggestion earlier in the thread. |
I agree, but I think it should be a separate pull request, since it's a bigger change |
Aye. That's why for the PR to get it in, it should hold some |
I think that change literally only requires touching two lines (or almost). |
Since we're discussing |
I believe it was to prevent overflow sum of |
Yeah, for some types |
To fix the |
Another incidental question: do we need to parametrize the weights by the type of |
This is fully implemented AFAICT. |
I would like to discuss adding a new type of weights for consistency and efficiency purposes. The proposed type is basically UnitWeights.
The text was updated successfully, but these errors were encountered: