Simplify to_constant_volume_bins#562
Conversation
|
|
||
| bins = [[]] if is_tuple_list else [{}] | ||
| else: | ||
| weights = sorted(items, key=lambda x: -x) |
There was a problem hiding this comment.
all of the above logic to get this reverse weights list is simplified to entries.sort(key=lambda e: -e[0]) below
| if not is_dict and not is_tuple_list and isinstance(items[0], (tuple, list)): | ||
| raise ValueError("Must provide weight_pos or key for tuple list") | ||
|
|
||
| weights = _get_bins(weights, valid_ndcs) |
There was a problem hiding this comment.
The above logic to get all of the valid indices is simplified to:
if lower_bound is not None:
entries = [e for e in entries if e[0] > lower_bound]
if upper_bound is not None:
entries = [e for e in entries if e[0] < upper_bound]
| weights = _get_bins(weights, valid_ndcs) | ||
|
|
||
| if isinstance(items, dict): | ||
| keys = _get_bins(keys, valid_ndcs) |
There was a problem hiding this comment.
this is no longer needed since we operate on the new tuple objects, which automatically carries the keys in the entries list
| for weight, payload in entries: | ||
| # get all bin indices that can fit this payload | ||
| candidate_bin_indices = [ | ||
| i for i, s in enumerate(bin_sums) if s + weight <= max_volume |
There was a problem hiding this comment.
one thing I noticed that was missing in the original implementation (I didn't add it here). but I think we need to have an assert to ensure that all weights are <= max_volume? bc if there contains an item that exceeds max_volume, we can never fit it into a bin, even a new bin
There was a problem hiding this comment.
maybe we add this line around 120?
# warn on oversized items — they'll still be placed in a bin by themselves,
# but that bin will exceed max_volume
for weight, payload in entries:
if weight > max_volume:
logger.warning(
"item %r has weight %s > max_volume %s; placing in its own bin anyway",
payload,
weight,
max_volume,
)Right now, we allow items to overflow the max bin size. some tests fail if we add this line and throw an exception
There was a problem hiding this comment.
I actually wanted this to be an exception, but I can see cases where people might want the weight to exceed the max volume so maybe it's not necessary to add this in.
curtischong
force-pushed
the
clean-to_constant_volume_bins
branch
from
32b395a to
10b47c4
Compare
clean fn cleanup ruff add back len check manual comments backup better logic add max volume check remove fix ruff
curtischong
force-pushed
the
clean-to_constant_volume_bins
branch
from
10b47c4 to
0341adb
Compare
curtischong
changed the title
1 participant
Summary
By standardizing all of the different formats into a single datatype before running the algorithm, we no longer need isinstance checks, which simplifies the algorithm (since the algorithm is only handling one type of object)
Checklist
Before a pull request can be merged, the following items must be checked: