-
Notifications
You must be signed in to change notification settings - Fork 15
Description
System setup
OS: [e.g] macOS v10.14.1
Python version: v3.6.10
Python environment (if any): [e.g.] conda v4.5.11
Describe the bug
I was recently moving from old to new version of modopt for some of other packages that use modopt.
I see the error on metrics:
I believe the error is coming from here:
ModOpt/modopt/opt/algorithms.py
Lines 146 to 154 in 616c9e7
| @metrics.setter | |
| def metrics(self, metrics): | |
| if isinstance(metrics, type(None)): | |
| self._metrics = {} | |
| elif not isinstance(metrics, dict): | |
| raise TypeError( | |
| 'Metrics must be a dictionary, not {0}.'.format(type(metrics)), | |
| ) |
We don't have any else! if metrics is dictionary but not None, then we dont set any metrics! ie. if metrics = {}, which is default.
I am quite surprised how testing missed this ! I might be missing something here surely.
I checked the reason for most of the codes to work in test is cause we dont test the metrics, and also the SetUp by default gives None as input to metrics.
So, effectively, modopt would fail for any algorithm with metrics defined as a dictionary (which is how it must be used)!!
To Repro
from modopt.opt.algorithms import SetUp
A = SetUp(metrics={})Results in :
AttributeError: 'SetUp' object has no attribute '_metrics'
To fix
we just need to add an else self._metrics = metrics line. However, @sfarrens how did we miss this in the refactoring?
@sfarrens, In my opinion this needs to be fixed at highest priority and the current release on PyPi must be removed and updated
with this. Perhaps, adding a dummy test with metrics is also very much needed!
Are you planning to submit a Pull Request?
- Yes
- No