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

An alternative to the "return_indices" parameter #4

Open
erelsgl opened this issue Nov 1, 2021 · 2 comments
Open

An alternative to the "return_indices" parameter #4

erelsgl opened this issue Nov 1, 2021 · 2 comments

Comments

@erelsgl
Copy link

erelsgl commented Nov 1, 2021

Most functions now have a boolean "return_indices" flag. As an alternative, and to simplify the interface, I suggest that the returned PartitioningResult object always contains the indices, and a pointer to the actual values. The object will have two methods: one to return the indices, and one to compute and return the values. This will simplify the functions, and move the decision of output format to the output object. For example:

result = greedy([11,22,33,44], 2)
print(result.indices())
print(result.values())
@fuglede
Copy link
Owner

fuglede commented Dec 21, 2021

For what it's worth, the idea with keeping it all as a simple function API protocol was that it tends to be a bit simpler to use; it also makes it a bit simpler if we include compiled versions of the functions a la Cython, Numba, Pythran, PyO3 (although that's hardly an argument as it'd be easy enough to wrap), and that you can save a bit of calculation/lookups if you specify which one you want up front (although that's all O(n) so not costly).

One thing I don't like about how it's set up right now is that it breaks the semantics. I wouldn't have done something like return_indices if it changed the return type of the function, and it would do that if the input values were, say, of type float, which makes return_indices a huge smell.

All in all, I think what you propose makes sense.

@fuglede
Copy link
Owner

fuglede commented May 17, 2022

What I converged to elsewhere is having the method itself have two parameters instead of one: the float-valued weights, and a generic list of elements to be split into parts, with a separate method for the common case where the elements are range(len(weights)).

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