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

Convert node- and product-related lists to sets #148

Closed
LarrySnyder opened this issue May 9, 2024 · 3 comments
Closed

Convert node- and product-related lists to sets #148

LarrySnyder opened this issue May 9, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@LarrySnyder
Copy link
Owner

e.g., raw_materials_by_product() etc should return sets, for speed

@LarrySnyder LarrySnyder added the enhancement New feature or request label May 9, 2024
@LarrySnyder LarrySnyder self-assigned this May 9, 2024
@LarrySnyder
Copy link
Owner Author

Also node predecessors, etc.

@LarrySnyder
Copy link
Owner Author

LarrySnyder commented May 9, 2024

This will be more involved than I first thought. We can't take sets of SupplyChainNode or other objects, because sets can only contain immutable objects. So I think this will require (using _predecessors as an example):

  • rename _predecessors to _predecessor_indices and make it a set of indices, not objects
  • change predecessor_indices property so it draws directly from _predecessor_indices -- it will return a set
  • change predecessors property so it obtains objects from indices, i.e., using SupplyChainNetwork.get_node_from_index() -- it will return a list
  • look through code to err on the side of using predecessor_indices, not predecessors, whenever possible

See https://eng.lyft.com/hashing-and-equality-in-python-2ea8c738fb9d for a nice summary of the trouble with using custom objects in sets. (The approach I'm considering above doesn't convert them to immutables, though, it just uses indices instead of objects in the sets.)

@LarrySnyder LarrySnyder added the coming soon Plan to do this in a not-too-distant release label May 10, 2024
LarrySnyder added a commit that referenced this issue May 23, 2024
@LarrySnyder LarrySnyder changed the title Convert product-related lists to sets Convert node- and product-related lists to sets May 24, 2024
@LarrySnyder
Copy link
Owner Author

This is not really a good idea. Lists are faster to iterate over than sets. Sets are faster to check membership but that's not the primary use case.

@LarrySnyder LarrySnyder added wontfix This will not be worked on and removed coming soon Plan to do this in a not-too-distant release labels May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant