Fix fit2 - #293
Conversation
Summary of ChangesHello @michaelteboul, 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 addresses a bug in the 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 addresses a crash within the fit method during the validation step. While the change successfully prevents the crash, it introduces a logical error by mislabeling validation loss as 'Validation Metrics' and ignoring the actual metrics that were computed. My review points out this flaw and suggests a refactoring approach to correctly process and report both validation loss and custom metrics.
| if metrics is not None: | ||
| print("Validation Metrics:", list(val_loss.values())[0].numpy()) | ||
| desc += ", Val Metrics " + ", ".join( | ||
| str(np.round(float(v), 4)) | ||
| for v in val_loss | ||
| if isinstance(v, (int, float, np.floating)) | ||
| ) | ||
| history["val_metrics"] = history.get("val_metrics", []) + [ | ||
| list(val_loss.values())[0] | ||
| ] | ||
| print("Validation Metrics:", val_loss.numpy()) | ||
| desc += f", Val Metrics {np.round(float(val_loss.numpy()), 4)}" | ||
| history["val_metrics"] = history.get("val_metrics", []) + [val_loss.numpy()] |
There was a problem hiding this comment.
While this change fixes a crash, it introduces a logical issue. The code now prints the validation loss under the label "Validation Metrics", which is misleading. The actual metrics computed by self.evaluate() on line 772 are calculated, but their results are then overwritten and never used.
The root cause is that the val_loss variable is first assigned the dictionary of metrics from evaluate(), but is then overwritten with the validation loss tensor on lines 817-819.
A more correct approach would be to refactor this part of the fit method to handle metrics and validation loss separately. Here's a conceptual example of how it could be structured:
# In fit method, inside `if val_dataset is not None:`
val_metrics_results = None
if metrics is not None:
# 1. Evaluate metrics and store them
val_metrics_results = self.evaluate(val_dataset, metrics=metrics)
# 2. Always calculate validation loss for history and callbacks
val_losses = []
for batch_nb, (...) in enumerate(val_dataset.iter_batch(...)):
# ... append to val_losses
# ... calculate final val_loss from val_losses
# 3. Log results
if verbose > 1:
if val_metrics_results:
# Log the actual metrics from val_metrics_results
metrics_str = ", ".join([f'{k}: {v.numpy():.4f}' for k, v in val_metrics_results.items()])
print(f"Validation Metrics: {metrics_str}")
desc += f", Val Metrics: {metrics_str}"
history["val_metrics"].append(val_metrics_results)
else:
# Log validation loss if no other metrics
print(f"Test Negative-LogLikelihood: {val_loss.numpy():.4f}")
desc += f", Test Loss {val_loss.numpy():.4f}"
# 4. Store validation loss in history
history["val_loss"].append(val_loss.numpy())This would ensure that when metrics are provided, they are correctly computed and reported, without being confused with the validation loss.
Coverage Report for Python 3.10
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.11
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Python 3.12
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2 participants
Description of the goal of the PR
Description:
Fix evaluate in fit
Checklist before requesting a review