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

Add reusable counterpart of WeightedIndex #1459

Open
maratik123 opened this issue Jun 15, 2024 · 4 comments
Open

Add reusable counterpart of WeightedIndex #1459

maratik123 opened this issue Jun 15, 2024 · 4 comments

Comments

@maratik123
Copy link

maratik123 commented Jun 15, 2024

Background

What is your motivation?
There is not any effective WeightedIndex implementation, that does not require new allocation on heap to replace all weighted index values. WeightedIndex::update_weights is only effective on updating small amount of weights.

What type of application is this?
Tasks such as Travelling salesman problem for their heuristic and approximation solutions (for example, via Ant colony optimization) require single sampling on each of multiple instances of WeightedIndex with different weights.

Feature request

In commited pull request #1460 I'm trying to add ReusableWeightedIndex with reusable storage to prevent unnecessary allocations

@dhardy
Copy link
Member

dhardy commented Jun 18, 2024

Is there a reason this can't be a new method like WeightedIndex::replace_weights instead of a whole new distribution type?

The main limitations of adding this functionality to WeightedIndex that I can see:

  • Construction with extra capacity must yield a valid distribution object (but it should be possible to do this in WeightedIndex)
  • If, during fill, there is an error, then the result may be left in an invalid state. We would need to handle this somehow.

The biggest limitation of your current approach that I see:

  • fill returns a temporary object referring to another object; this will be difficult to store in a struct (requiring ouroboros or similar)

@maratik123
Copy link
Author

Is there a reason this can't be a new method like WeightedIndex::replace_weights instead of a whole new distribution type?

The main limitations of adding this functionality to WeightedIndex that I can see:

  • If, during fill, there is an error, then the result may be left in an invalid state. We would need to handle this somehow.

Yes, this is the main blocker, why I decided to make new implementation. I can not see how to make replace_weights as cheap as possible with such restriction.

The biggest limitation of your current approach that I see:

  • fill returns a temporary object referring to another object; this will be difficult to store in a struct (requiring ouroboros or similar)

I don't wait that this one-time object will require to store in other structs, because it's main target to create weighting fast, make one sampling and drop. If it is required to store object in other struct, then one can use WeightedIndex instead.

@dhardy
Copy link
Member

dhardy commented Jun 20, 2024

Understood, however I have concerns:

  • This is model fairly specialised for your application
  • Having three variants of weighted-index samplers feels excessive — an indicator that we might want to rethink the API.

@vks
Copy link
Collaborator

vks commented Jun 20, 2024

Couldn't we just implement WeightedIndex::replace_weights(self, weights: I) -> Result<WeightedIndex> or similar?

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

3 participants