ADD: auto handling of 'end-of-basket' item for Shopper model #271
Conversation
Summary of ChangesHello @VincentAuriau, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural change to the Shopper model by implementing explicit and automatic handling of an 'end-of-basket' item. This refactoring ensures that the model consistently recognizes and processes the termination of a basket as a distinct event, impacting how item embeddings are managed, how data is augmented, and how item likelihoods are computed. The changes aim to improve the model's accuracy and robustness by providing a dedicated representation for the end-of-basket state. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Shopper model to automatically handle an 'end-of-basket' item, which is a significant improvement. The changes are mostly well-implemented, separating the end-of-basket embeddings and updating the data generation and utility computation logic accordingly.
I've found a few areas for improvement:
- There's a critical bug in
compute_item_likelihoodwhere the end-of-basket item is made unavailable, which would prevent it from ever being chosen. - Some debugging
printstatements have been left in the code. - There are opportunities to reduce code duplication in
basket_dataset.pyandshopper.pyto improve maintainability.
My detailed comments are below.
| if len(prices) == self.n_items: | ||
| prices = np.concatenate([prices, [0.0]], axis=0) | ||
| if len(available_items_copy) == self.n_items: | ||
| available_items_copy = np.concatenate([available_items_copy, [0.0]], axis=0) |
There was a problem hiding this comment.
The end-of-basket item's availability is being set to 0.0. This will prevent it from ever being chosen, as softmax_with_availabilities will multiply its probability by zero. Based on the logic elsewhere (e.g., in basket_dataset.py), it should be available for selection, so this should be 1.0.
| available_items_copy = np.concatenate([available_items_copy, [0.0]], axis=0) | |
| available_items_copy = np.concatenate([available_items_copy, [1.0]], axis=0) |
| print(batch_size, available_item_batch.shape, item_batch.shape) | ||
| print(basket_batch.shape, future_batch.shape, item_batch.shape) | ||
| print(available_item_batch[batch_size - 1]) |
There was a problem hiding this comment.
These print statements appear to be for debugging purposes. They should be removed before merging to keep the output clean and avoid potential performance issues.
| if data_method == "shopper": | ||
| buffer = ( | ||
| np.empty(0, dtype=int), # Items | ||
| np.empty((0, self.max_length), dtype=int), # Baskets | ||
| np.empty((0, self.max_length), dtype=int), # Future purchases | ||
| np.empty(0, dtype=int), # Stores | ||
| np.empty(0, dtype=int), # Weeks | ||
| np.empty((0, self.n_items + 1), dtype=int), # Prices | ||
| np.empty((0, self.n_items + 1), dtype=int), # Available items | ||
| ) | ||
| elif data_method == "aleacarta": | ||
| buffer = ( | ||
| np.empty(0, dtype=int), # Items | ||
| np.empty((0, self.max_length), dtype=int), # Baskets | ||
| np.empty((0, self.max_length), dtype=int), # Future purchases | ||
| np.empty(0, dtype=int), # Stores | ||
| np.empty(0, dtype=int), # Weeks | ||
| np.empty((0, self.n_items), dtype=int), # Prices | ||
| np.empty((0, self.n_items), dtype=int), # Available items | ||
| ) | ||
| else: | ||
| raise ValueError(f"Unknown data method: {data_method}") |
There was a problem hiding this comment.
There's some code duplication in the initialization of the buffer. The first five elements of the tuple are identical for both shopper and aleacarta data methods. Consider refactoring this to improve maintainability by defining the common part of the buffer first, and then appending the method-specific parts based on data_method.
| # end-of-basket rho | ||
| self.rho_eob = tf.Variable( | ||
| tf.random_normal_initializer(mean=0, stddev=1.0, seed=42)( | ||
| shape=(1, self.latent_sizes["preferences"]) | ||
| ), # Dimension for 1 item: latent_sizes["preferences"] | ||
| trainable=True, | ||
| name="rho_eob", | ||
| ) | ||
| self.alpha = tf.Variable( | ||
| tf.random_normal_initializer(mean=0, stddev=1.0, seed=42)( | ||
| shape=(n_items, self.latent_sizes["preferences"]) | ||
| ), # Dimension for 1 item: latent_sizes["preferences"] | ||
| trainable=True, | ||
| name="alpha", | ||
| ) | ||
| self.alpha_eob = tf.Variable( # end-of-basket alpha | ||
| tf.random_normal_initializer(mean=0, stddev=1.0, seed=42)( | ||
| shape=(1, self.latent_sizes["preferences"]) | ||
| ), # Dimension for 1 item: latent_sizes["preferences"] | ||
| trainable=True, | ||
| name="alpha_eob", | ||
| ) |
There was a problem hiding this comment.
The initialization of embedding variables like rho, alpha, and their _eob counterparts is quite repetitive. You could introduce a private helper method to create these tf.Variable embeddings. This would reduce code duplication and make the instantiate method cleaner and easier to maintain. For example:
def _create_embedding(self, name, shape, stddev, latent_size_key, mean=0.0):
initializer = tf.random_normal_initializer(mean=mean, stddev=stddev, seed=42)
return tf.Variable(
initializer(shape=(shape, self.latent_sizes[latent_size_key])),
trainable=True,
name=name,
)
# In instantiate method:
self.rho = self._create_embedding("rho", n_items, 1.0, "preferences")
self.rho_eob = self._create_embedding("rho_eob", 1, 1.0, "preferences")
# ... and so on for other embeddings
Coverage Report for Python 3.10
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.11
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.12
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2 participants
Shopper's 'end-of-basket' is now handled differently: