feature/samples_summary_failsafe#1168
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the instance property in the SearchOutput class to improve error handling when the max_log_likelihood method is unavailable. Instead of returning None, it now attempts to fall back to samples_summary.instance as an alternative source for the instance data.
Changes:
- Modified exception handling in the
instanceproperty to returnself.samples_summary.instanceinstead ofNonewhenAttributeErrororNotImplementedErroris raised
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return self.samples.max_log_likelihood() | ||
| except (AttributeError, NotImplementedError): | ||
| return None | ||
| return self.samples_summary.instance |
There was a problem hiding this comment.
This change introduces a potential AttributeError that is not caught. The samples_summary property (line 227) calls self.value("samples_summary") which can return None if the file doesn't exist. When summary is None, line 228 will fail with AttributeError: 'NoneType' object has no attribute 'model'. Additionally, even if samples_summary is not None, accessing its instance property could raise exceptions since it internally calls max_log_likelihood().
The docstring on line 248 states "None if samples cannot be loaded", but the new code path doesn't handle these potential failures. Consider wrapping the fallback in a try-except block similar to the pattern used in autofit/non_linear/result.py:115-120, or add proper null checking.
This pull request updates the behavior of the
instancemethod inautofit/aggregator/search_output.pyto improve handling of cases where themax_log_likelihoodmethod is unavailable. Instead of returningNone, it now falls back to returning the instance fromsamples_summary.Error handling improvement:
instancemethod to returnself.samples_summary.instancewhenmax_log_likelihoodis not implemented or accessible, rather than returningNone.