Add Gaussian confidence level attribute to PyROS EllipsoidalSet#3434
Add Gaussian confidence level attribute to PyROS EllipsoidalSet#3434mrmundt merged 13 commits intoPyomo:mainfrom
EllipsoidalSet#3434Conversation
shermanjasonaf
changed the title
EllipsoidalSetEllipsoidalSet
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Two ignorable minor comments, but otherwise looks good.
| if scale is not None and gaussian_conf_lvl is None: | ||
| self.scale = scale | ||
| elif scale is None and gaussian_conf_lvl is not None: | ||
| self.gaussian_conf_lvl = gaussian_conf_lvl | ||
| else: | ||
| raise ValueError( | ||
| "Exactly one of `scale` and `gaussian_conf_lvl` should be " | ||
| f"None (got {scale=}, {gaussian_conf_lvl=})" | ||
| ) |
There was a problem hiding this comment.
You don't need to change this, but would it be more clear to have this be:
if (scale is None) ^ (gaussian_conf_lvl is None):
self.scale = scale
self.gaussian_conf_lvl = gaussian_conf_lvl
else:
raise ValueError(
"Exactly one of `scale` and `gaussian_conf_lvl` should be "
f"None (got {scale=}, {gaussian_conf_lvl=})"
)And then have the setters just "do nothing" if the incoming value is None?
There was a problem hiding this comment.
I agree this would clarify the code, but are we sure we want each of those setters to leave the state unchanged if None is passed (without at least noting this behavior in the property docstrings)? Currently, each of the other EllipsoidalSet attribute setters (and more broadly, attribute setters for other concrete UncertaintySet subclasses) raises an exception if None is passed and not an acceptable value/type/shape for the attribute. I am wondering whether this new behavior for scale and gaussian_conf_lvl might not be clear to users attempting to set either attribute to None post construction.
…f/pyomo into pyros-confidence-ellipsoid
| np.testing.assert_allclose( | ||
| sp.stats.chi2.isf(q=1 - init_conf_lvl, df=eset.dim), | ||
| eset.scale, | ||
| err_msg="EllipsoidalSet scale not as expected", |
There was a problem hiding this comment.
Suggestion: I won't have this hold up the PR, but it might be worth making this message into its own class-level var so you don't have to potentially change the error message in a bunch of places if you decide to modify the language or the error message someday.
mrmundt
left a comment
mrmundt
left a comment
There was a problem hiding this comment.
I am fine with this PR as-is, but I did have one suggestion for better maintainability within the tests.
4 participants
Summary/Motivation:
Motivated by recent user feedback, this PR adds a new PyROS$q^0 \in \mathbb{R}^n$ , positive definite shape matrix $P \in \mathbb{S}_{++}^n$ , and squared semi-axis scale factor $s \geq 0$ is defined by
EllipsoidalSetattribute and constructor argumentgaussian_conf_lvlthat specifies, as an alternative to the the squared semi-axis scaling factor attribute and constructor argumentscale, the level of the (ellipsoidal) confidence region of the multivariate Gaussian distribution with meancenterand covarianceshape_matrix. Mathematically, the PyROS ellipsoidal set with centerFor confidence level$p \in [0, 1)$ , the $100p$ % multivariate Gaussian confidence region is obtained by setting $s = \chi_{n,p}^2$ , where $\chi_{n,\cdot}^2$ is the (invertible) lower-tail quantile function of the chi-squared distribution with $n$ degrees of freedom. Thus, we ensure that setting $s$ and confidence level $p$ , an update of
gaussian_conf_lvltriggers, through the invertible mapping between the squared scaling factorscale, and vice-versa.Changes proposed in this PR:
gaussian_conf_lvlto PyROSEllipsoidalSet.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: