Raise error on failed Param validation

[goroderickgo]

Fixes #930

Summary/Motivation:

As noted in #930, the validate option for Params does not raise an error when the validation function returns False (i.e., the functionality described in the documentation does not work as described).

Changes proposed in this PR:

• Raise the error that was caught in the try/except block. This proposed fix is modeled after the pattern used for the methods with the same name (_setitem_when_not_present) in for IndexedComponent and Var.

I'm not very familiar with adding tests, so I would gladly take guidance if I need to add any.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[codecov]

Codecov Report

Merging #1272 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1272      +/-   ##
==========================================
- Coverage   70.93%   70.93%   -0.01%     
==========================================
  Files         534      534              
  Lines       80964    80965       +1     
==========================================
- Hits        57431    57429       -2     
- Misses      23533    23536       +3

Impacted Files	Coverage Δ
pyomo/core/base/param.py	80.74% <0%> (-0.22%)	⬇️
pyomo/pysp/ph.py	61.93% <0%> (-0.1%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69b2e7b...9969ca4. Read the comment docs.

[blnicho]

I tested this locally and it seems to fix things. I don't think this will have any unintended side effects but we should wait for @jsiirola to take a look before merging.

For tests, take a look at https://github.com/Pyomo/pyomo/blob/master/pyomo/core/tests/unit/test_param.py. I think we just need to add a method under MiscParamTests that declares a few different types of Params (indexed/nonindexed, mutable/immutable) and asserts that a ValueError is raised when the Param is set to an invalid value. The syntax for that is something like:

with self.assertRaises(ValueError):
    m.p = -3

[goroderickgo]

Thanks @blnicho! Agreed on waiting for @jsiirola, just figured I’d take a stab at fixing the issue. I’ll take a look at the tests soon.

[jsiirola]

I think this solution looks fine, and will approve once we have some tests to make sure that this doesn't sneak back in. To add to what @blnicho said, I generally prefer that we verify the exception message as well as the type (especially when we are looking for a common type like ValueError) - I have gotten burned before by just looking at the type: the test was passing because the right type was being raised, but it wasn't the exception I was expecting.

In this case, something like

with self.assertRaisesRegexp(ValueError, "Value failed parameter validation rule"):
    m.p = -3

would be good.

[goroderickgo]

Hi everyone, I could use a bit more guidance on the tests:

1. If we initialize a Param like so: m.x = Param(initialize=-3, within=NonNegativeReals), it also raises a ValueError, but it's wrapped in a more generic "ERROR" that doesn't pass the validation test. Seems like I should be testing for this, but I'm not sure how, since "ERROR" is not a defined type of exception.

   ERROR: Constructing component 'p1' from data=None failed:
           ValueError: Invalid parameter value: p1[None] = '-3', value
           type=<class 'int'>.
           Value not in parameter domain NonNegativeReals
   

2. I'm on Python 3.8 and am getting a deprecation warning for self.assertRaisesRegexp (see below). Should I use self.assertRaisesRegex as it suggests instead?

   DeprecationWarning: Please use assertRaisesRegex instead.
   

Thanks!

[jsiirola]

@goroderickgo: for Q1, while Block (and Model) will catch the ValueError exception and log additional context, the original exception is re-raised and you can catch it as normal. So, something like the following should work:

with self.assertRaisesRegexp(ValueError, "Value not in parameter domain NonNegativeReals"):
    m.p = Param(initialize=-3, domain=NonNegativeReals)

For Q2: until PyUtilib/pyutilib#79 is accepted and merged in, using assertRaisesRegex will cause Python 2.7 tests to fail (which would prevent merging this PR). My recommendation would be patience - I expect that PR to be reviewed and merged in about a day. After that, using assertRaisesRegex will be the preferred syntax for all of Pyomo.

[blnicho]

@goroderickgo I merged the PyUtilib PR so you should be fine to move ahead with the assertRaisesRegex syntax.