-
Notifications
You must be signed in to change notification settings - Fork 863
[Bug] Incorrect default assignment logic for n_plotting_samples in DeepAR #2246
Description
Hi there,
I recently noticed a potential inconsistency in how n_plotting_samples is initialized in the DeepAR model. Based on the docstring, n_plotting_samples is expected to default to n_validation_samples if it's provided, or to 100 otherwise.
However, the current implementation in pytorch_forecasting/models/deepar/_deepar.py seems to have this logic inverted:
if n_plotting_samples is None:
if n_validation_samples is None:
n_plotting_samples = n_validation_samples # Sets to None
else:
n_plotting_samples = 100 # Sets to 100, regardless of n_validation_samplesExpected Behavior
If the docstring is correct, the logic should probably look like this:
if n_plotting_samples is None:
n_plotting_samples = n_validation_samples if n_validation_samples is not None else 100Why this matters
- Consistency: The current behavior deviates from the documented parameters, which can be confusing for users expecting their plotting sample size to match their validation sample size.
- Robustness: If
n_plotting_samplesends up asNone(whenn_validation_samplesisNone), it might cause issues in plotting utilities that expect an integer value.
Minimal Reproduction
A quick simulation of the current logic shows the mismatch:
def simulate_init(n_validation_samples=None, n_plotting_samples=None):
if n_plotting_samples is None:
if n_validation_samples is None:
n_plotting_samples = n_validation_samples
else:
n_plotting_samples = 100
return n_plotting_samples
print(f"Provided n_validation_samples=10: Resulting n_plotting_samples={simulate_init(10)}") # Returns 100
print(f"Provided n_validation_samples=None: Resulting n_plotting_samples={simulate_init(None)}") # Returns NoneSuggested Resolution
Update the initialization in DeepAR to align with the docstring:
if n_plotting_samples is None:
n_plotting_samples = n_validation_samples if n_validation_samples is not None else 100Before proceeding with a PR, it would be helpful to confirm:
Whether the docstring reflects the intended behavior
Or if the current implementation is correct and the docstring needs updating instead
Happy to open a PR once the intended behavior is clarified.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status