openmmtools icon indicating copy to clipboard operation
openmmtools copied to clipboard

Avoid raising broad `Exception` when raising errors

Open ijpulidos opened this issue 1 year ago • 3 comments

Some check and errors are raised with generic Exception objects, this can lead to some troubles in the future even if we are safe now.

We should enhance our code base and create and raise specific exceptions when dealing with errors.

As a first pass I detected the following parts in the code (not tests) where we may be facing this issue:

./openmmtools/alchemy.py:        raise Exception("Not implemented; needs CustomMultipleForce")
./openmmtools/multistate/multistatesampler.py:                raise Exception('Thermodynamic states contain a mixture of '
./openmmtools/multistate/multistatesampler.py:                    raise Exception('All sampler states must have box_vectors defined if the system is periodic.')
./openmmtools/multistate/multistatesampler.py:            raise Exception('Cannot use MBAR with non-global locality.')
./openmmtools/multistate/sams.py:                raise Exception('locality must be >= 1')
./openmmtools/multistate/sams.py:                raise Exception('Initial logZ_guess (dim {}) must have same number of states as n_states ({})'.format(
./openmmtools/multistate/sams.py:                raise Exception('Programming error: Unreachable code')
./openmmtools/multistate/sams.py:                raise Exception('stage {} unknown'.format(self._stage))
./openmmtools/multistate/sams.py:                raise Exception('Programming error: Unreachable code')
./openmmtools/multistate/multistateanalyzer.py:            raise Exception('Non-global MBAR analysis not implemented yet.')
./openmmtools/multistate/multistateanalyzer.py:            raise Exception("Cannot specify statistical_inefficiency without n_equilibration_iterations, because " \
./openmmtools/integrators.py:            raise Exception('This integrator is not a CustomIntegrator.')
./openmmtools/integrators.py:            raise Exception("Invalid Yoshida-Suzuki value. Allowed values are: %s"%
./openmmtools/integrators.py:            raise Exception("Nosé-Hoover chain length must be at least 0")
./openmmtools/integrators.py:            raise Exception("This integrator must be constructed with 'measure_shadow_work=True' to measure shadow work.")
./openmmtools/integrators.py:            raise Exception("This integrator must be constructed with 'measure_heat=True' in order to measure heat.")
./openmmtools/integrators.py:            raise Exception("This integrator must be Metropolized to return an acceptance rate.")
./openmmtools/integrators.py:            raise Exception('nsteps_neq must be an integer >= 0')
./openmmtools/integrators.py:            raise Exception("alpha must be in the interval (0,1); specified alpha = %f" % alpha)
./openmmtools/utils.py:        raise Exception('Argument is not a type')
./openmmtools/utils.py:    raise Exception(f"Platform {platform.getName()} unknown")

ijpulidos avatar Aug 01 '22 21:08 ijpulidos

I think that unless we have some clear-cut ones that could use further classification, making these OpenmmtoolsError or OpenMMToolsError is probably good enough.

mikemhenry avatar Aug 01 '22 22:08 mikemhenry

Also see #523

I was bit by this again recently and came here to make the issue :)

mikemhenry avatar Aug 17 '22 00:08 mikemhenry

Good catch! Pretty much all of these should be ValueError.

jchodera avatar Aug 17 '22 13:08 jchodera