ADD: Latent-Class model on preference cars dataset - #327
Conversation
VincentAuriau
commented
Jul 7, 2026
VincentAuriau
commented
- small improvements
![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 introduces features to load car preferences with one-hot encoded categorical features or specific preprocessing in choice_learn/datasets/base.py, adds seed-based initialization to latent class and simple MNL models, and refactors the training and EM fit logic in latent_class_base_model.py. However, the review highlights several critical issues in the model changes: the new probability and weight calculations in latent_class_base_model.py are mathematically incorrect, contain redundant/dead code, and omit latent class weights from trainable_weights. Additionally, initializing self.weights in fit causes collisions and shape mismatches, clipping probabilities to 0.0 introduces numerical instability, and commenting out EM initialization degrades convergence. Finally, the review recommends vectorizing the DataFrame operations in the dataset loader and avoiding global seed modification inside model instantiation methods.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| sample_class_weight = tf.stack(probabilities, axis=1) / tf.reduce_sum( | ||
| tf.stack(probabilities, axis=1), axis=1, keepdims=True | ||
| ) | ||
| sample_class_weight = tf.stack( | ||
| [ | ||
| tf.gather_nd(prob, tf.stack([np.arange(0, len(choices)), choices], axis=1)) | ||
| for prob in probabilities | ||
| ], | ||
| axis=1, | ||
| ) | ||
| # sample_class_weight = tf.expand_dims(tf.reduce_sum(sample_class_weight, | ||
| # axis=0, keepdims=True), axis=-1) | ||
| self._weights = sample_class_weight | ||
| self.__weights = tf.stack( | ||
| [ | ||
| tf.gather_nd(prob, tf.stack([np.arange(0, len(choices)), choices], axis=1)) | ||
| for prob in probabilities | ||
| ], | ||
| axis=1, | ||
| ) | ||
| # Summing over the latent classes | ||
| probabilities = tf.reduce_sum(probabilities, axis=0) | ||
|
|
||
| probabilities = tf.reduce_sum( | ||
| tf.stack(probabilities, axis=1) * tf.expand_dims(sample_class_weight, axis=-1), | ||
| axis=1, | ||
| ) |
There was a problem hiding this comment.
There are several critical issues in this block:
sample_class_weightis calculated on lines 563-565 and then immediately overwritten on lines 566-572, rendering the first calculation dead code.self.__weightsis computed on lines 576-582 using the exact same expression assample_class_weight, which is redundant.- The calculation of
probabilitieson lines 585-588 usessample_class_weight(the probability of the chosen item in each class) as the mixture weights. This is mathematically incorrect for computing the probability of choosing any item. It violates the latent class model formulation where the mixture weights must be the prior class probabilities (derived fromself.latent_logits), not the posterior/chosen item probabilities. This also results inprobabilitiesnot summing to 1 across items, which violates the requirements of the Categorical Cross-Entropy loss.
| weights = [] | ||
| # weights = [self.weights] | ||
| for model in self.models: | ||
| weights += model.trainable_weights | ||
| return weights |
There was a problem hiding this comment.
The latent class weights (either self.latent_logits or self.weights) are omitted from trainable_weights. This prevents the optimizer from updating the mixture weights during gradient descent (_fit_with_gd), meaning the class probabilities will remain static at their initial random values.
| self.weights = tf.Variable( | ||
| tf.random_normal_initializer(0.0, 0.08)( | ||
| shape=(self.n_latent_classes - 1, len(choice_dataset)) | ||
| ), | ||
| name="Latent-Logits", | ||
| ) |
There was a problem hiding this comment.
Initializing self.weights as a tf.Variable of shape (self.n_latent_classes - 1, len(choice_dataset)) in fit causes several issues:
- It collides with
self.weightsused in EM fit (_em_fit), whereself.weightsis a tensor of shape(len(choice_dataset), n_latent_classes)representing the posterior class probabilities. - In L-BFGS training (
_lbfgs_train_step),self.weightsis appended totrainable_weights, but in_fit_with_lbfgs,self.latent_logits(shape(n_latent_classes - 1,)) is appended toinit. This shape mismatch will causetf.dynamic_stitchto crash. - Having observation-specific latent parameters of shape
(n_latent_classes - 1, len(choice_dataset))without any sharing or regularization is extremely prone to overfitting and makes out-of-sample prediction impossible.
| for i in range(1, 7): | ||
| for car_type in ["regcar", "sportcar", "sportuv", "stwagon", "truck", "van"][:-1]: | ||
| cars_df[f"{car_type}{i}"] = cars_df.apply( | ||
| lambda row: row[f"type{i}"] == car_type, axis=1 | ||
| ).astype(int) | ||
| for i in range(1, 7): | ||
| for fuel_type in ["cng", "electric", "gasoline", "methanol"][:-1]: | ||
| cars_df[f"{fuel_type}{i}"] = cars_df.apply( | ||
| lambda row: row[f"fuel{i}"] == fuel_type, axis=1 | ||
| ).astype(int) |
There was a problem hiding this comment.
Using .apply(lambda row: ...) row-by-row on a pandas DataFrame is highly inefficient and slow. Since the operations are simple equality checks, they can be fully vectorized using pandas' native element-wise comparison, which is significantly faster.
| for i in range(1, 7): | |
| for car_type in ["regcar", "sportcar", "sportuv", "stwagon", "truck", "van"][:-1]: | |
| cars_df[f"{car_type}{i}"] = cars_df.apply( | |
| lambda row: row[f"type{i}"] == car_type, axis=1 | |
| ).astype(int) | |
| for i in range(1, 7): | |
| for fuel_type in ["cng", "electric", "gasoline", "methanol"][:-1]: | |
| cars_df[f"{fuel_type}{i}"] = cars_df.apply( | |
| lambda row: row[f"fuel{i}"] == fuel_type, axis=1 | |
| ).astype(int) | |
| for i in range(1, 7): | |
| for car_type in ["regcar", "sportcar", "sportuv", "stwagon", "truck", "van"][:-1]: | |
| cars_df[f"{car_type}{i}"] = (cars_df[f"type{i}"] == car_type).astype(int) | |
| for i in range(1, 7): | |
| for fuel_type in ["cng", "electric", "gasoline", "methanol"][:-1]: | |
| cars_df[f"{fuel_type}{i}"] = (cars_df[f"fuel{i}"] == fuel_type).astype(int) |
| return tf.clip_by_value( | ||
| predicted_probas / np.sum(predicted_probas, axis=1, keepdims=True), 1e-6, 1 | ||
| predicted_probas / np.sum(predicted_probas, axis=1, keepdims=True), 0.0, 1.0 | ||
| ), loss |
There was a problem hiding this comment.
Clipping the predicted probabilities to 0.0 instead of a small positive epsilon (like 1e-6 or 1e-15) can lead to numerical instability. Specifically, in the M-step (_maximization), tf.math.log is computed on the latent probabilities. If any class probability becomes exactly 0.0 due to this clipping, tf.math.log will receive 0.0 and return -inf or NaN, causing the training to fail.
| return tf.clip_by_value( | |
| predicted_probas / np.sum(predicted_probas, axis=1, keepdims=True), 1e-6, 1 | |
| predicted_probas / np.sum(predicted_probas, axis=1, keepdims=True), 0.0, 1.0 | |
| ), loss | |
| return tf.clip_by_value( | |
| predicted_probas / np.sum(predicted_probas, axis=1, keepdims=True), 1e-15, 1.0 | |
| ), loss |
| # init_sample_weight = np.random.rand(self.n_latent_classes, len(choice_dataset)) | ||
| # init_sample_weight = np.clip( | ||
| # init_sample_weight / np.sum(init_sample_weight, axis=0, keepdims=True), 1e-6, 1 | ||
| # ) | ||
| # for i, model in enumerate(self.models): | ||
| # # model.instantiate() | ||
| # model.fit(choice_dataset, sample_weight=init_sample_weight[i], verbose=verbose) |
There was a problem hiding this comment.
Commenting out the initialization of the latent models in EM fit means that the models will start with raw random weights instead of being pre-fitted on a random partition of the dataset. This can significantly degrade the convergence speed and the quality of the local optimum found by the EM algorithm. If this was intentional, please provide an alternative initialization scheme or document why it was removed.
| tf.random.set_seed(base_seed) | ||
| self.latent_logits = tf.Variable( | ||
| tf.random_normal_initializer(0.0, 0.02, seed=42)(shape=(self.n_latent_classes - 1,)), | ||
| tf.random_normal_initializer(0.0, 0.02)(shape=(self.n_latent_classes - 1,)), | ||
| name="Latent-Logits", | ||
| ) |
There was a problem hiding this comment.
Calling tf.random.set_seed(base_seed) inside a model's instantiation method is a bad practice in library code because it modifies the global TensorFlow random number generator state. This can have unintended side effects on other parts of the user's program or other models being instantiated concurrently/subsequently. It is better to pass the seed directly to the initializer (e.g., tf.random_normal_initializer(0.0, 0.02, seed=base_seed)) or use a stateful tf.random.Generator.
| list of tf.Tensor | ||
| List of the weights created coresponding to the specification. | ||
| """ | ||
| tf.random.set_seed(base_seed) |
There was a problem hiding this comment.
Calling tf.random.set_seed(base_seed) inside a model's instantiation method is a bad practice in library code because it modifies the global TensorFlow random number generator state. This can have unintended side effects on other parts of the user's program or other models being instantiated concurrently/subsequently. It is better to pass the seed directly to the initializer (e.g., tf.random_normal_initializer(0.0, 0.02, seed=base_seed)) or use a stateful tf.random.Generator.
Coverage Report for Python 3.9
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.11
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.12
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1 participant
