Update PyROS Uncertainty Set Validation Methods

[jas-yao]

Fixes: #2724, #3508

Summary/Motivation:

This PR provides updates to PyROS uncertainty set validation methods and related tests.
Here, a validate method replaces the is_valid method (which solves 2N bounding problems to check for set boundedness) in all uncertainty sets, with each set having its own custom validate method that efficiently checks set-specific attributes and raises informative exceptions if any issues are found.

Changes proposed in this PR:

• Update is_bounded and is_nonempty methods in base UncertaintySet class
• Provide a _solve_feasibility method in base UncertaintySet class
• Replace is_valid with validate method that runs is_bounded and is_nonempty in the base UncertaintySet class
• Override validate in subclass uncertainty sets to check set-specific attributes
• Remove attribute setter checks in uncertainty sets that have been moved to the validate method
• Update unit tests for is_bounded, is_nonempty, _solve_feasibility, and validate methods

TODO

• Add FBBT for _compute_parameter_bounds
• Add validate_array to all validate methods
• Split test_validate tests into separate tests for each validation check
• Delete _validate method for PolyhedralSet
• Update error messages to clearly indicate issues specific to uncertainty set setup
• Check uncertainty set documentation looks good and test fix for Invalid doctest in PyROS - unclear what behavior should be #3508
• Update version number and changelog (after Update the PyROS Separation Priority Ordering Interface #3581 merged)

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:

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.

[blnicho]

@jas-yao is this ready for review or should we wait for the TODO's in the description to be completed?

[jas-yao]

Hi @blnicho, I will need to finish some of the TODOs before this will be ready for review.

[blnicho]

@jas-yao I'm going to convert this to a draft until it is ready for review.

[shermanjasonaf]

@jas-yao Please delay the version number/changelog update until after #3581 has been merged.

[jas-yao]

@shermanjasonaf, @blnicho, @jsiirola I have completed the TODOs and think this PR should be ready for review now.

[jas-yao]

I noticed that this test (check that the new is_bounded method is faster than the old 2N optimization problems) is failing in some cases. Specifically:

AssertionError: 0.0 not less than 0.0 : Boundedness check with provided parameter_bounds took longer than expected.

I realize this timing test is not the best due to different runtimes on different systems and was wondering if I should just remove this? Using assertLessEqual may also resolve the issue.

[jsiirola]

Over all, this looks pretty good. A couple small questions / edits and I think we can be good to go.

[jsiirola]

Instead if "renaming" native_numeric_types, it would be better to just reference that set directly. In particular, it will help developers who are familiar with its use elsewhere to recognize what you are doing here.

[jsiirola]

We should probably update _compute_parameter_bounds so that any (partially known) bounds (from FBBT) can be skipped (and not re-solved for)?

Also, when looking at _compute_parameter_bounds:

        if index is None:
            index = list(range(self.dim))
        bounding_model = self._create_bounding_model()
        objs_to_optimize = (
            (idx, obj)
            for idx, obj in bounding_model.param_var_objectives.items()
            if idx in index
        )

This i very inefficient (it is a quadratic search). I would recommend rewriting it as

        bounding_model = self._create_bounding_model()
        objs_to_optimize = bounding_model.param_var_objectives.items()
        if index is not None:
            set_of_target_indices = set(index)
            objs_to_optimize = filter(lambda idx, obj: idx in set_of_target_indices, objs_to_optimize)

[shermanjasonaf]

@jsiirola I agree that we should probably modify _compute_parameter_bounds() such that we can skip the global solution of bounding problems corresponding to finite bounds that were already obtained via FBBT. In so doing, we should take into consideration other methods that invoke _compute_parameter_bounds() and rely on the exactness of the bounds returned. One such method is _is_coordinate_fixed(), added in #3503.

A possible resolution:

1. Allow the optional index argument to _compute_parameter_bounds() to be None or either of the following:
   • A list (with length self.dim) of 2-tuple of bool, such that for each entry of each tuple, a value of True [False] indicates that the corresponding bounding problem should be solved [skipped]. In this case, make _compute_parameter_bounds() return a list of 2-tuples such that the entries corresponding to skipped bounds are of value None.
   • A list of 2-tuples corresponding to the bounding problems that should be solved. The first entry of each tuple indicates the ordinal position of the uncertain parameter and the second entry is a 0-1 value indicating the bound (lower or upper). In this case, make _compute_parameter_bounds() return a dict mapping the tuples to the calculated bounds.
2. Make use of the modified index argument to _compute_parameter_bounds() in is_bounded(), such that only bounds for which non-finite values were reported by the FBBT method are (re-)calculated.
3. Account for changes to the logic of the index argument to_compute_parameter_bounds() throughout the rest of the PyROS codebase. In particular, the method _is_coordinate_fixed() may need to be modified to account for changes to the structure of the returned bounds.

For clarity, we should probably also rename _compute_parameter_bounds() to _compute_exact_parameter_bounds() and/or ensure that the introductory one-sentence summary in the docstring explicitly conveys that the bounds returned are exact (that is, modify the sentence to "Compute exact lower and upper bounds...").

I would probably also ensure that the docstring of _fbbt_parameter_bounds() explicitly conveys that the bounds returned by _fbbt_parameter_bounds() are, or may be, inexact.

[jsiirola]

Why do you assign these to new variables? I think the code would be easier to read if you just:

Suggested change

	if not check_nonempty:
	raise ValueError(
	"Failed nonemptiness check. Nominal point is not in the set. "
	f"Nominal point:\n {config.nominal_uncertain_param_vals}."
	)
	if not self.is_nonempty(config=config):
	raise ValueError(
	"Failed nonemptiness check. Nominal point is not in the set. "
	f"Nominal point:\n {config.nominal_uncertain_param_vals}."
	)

[shermanjasonaf]

@jas-yao Overall, this PR looks good. I have several comments related to the documentation and logging/exception messages. A few tests need to be modified to ensure that all tests pass.

[shermanjasonaf]

Since a tuple is an iterable and validate_arg_type now performs no treatment specific to tuples:

Suggested change

	valid_types : type, tuple of types, or iterable of types
	valid_types : type or iterable of types

[shermanjasonaf]

Suggested change

	or a list of length = self.dim if it can.
	or a list of length ``self.dim`` if it can.

[shermanjasonaf]

Suggested change

	This method is invoked by validate.
	This method is invoked by ``self.validate()``.

[shermanjasonaf]

@jsiirola I agree that we should probably modify _compute_parameter_bounds() such that we can skip the global solution of bounding problems corresponding to finite bounds that were already obtained via FBBT. In so doing, we should take into consideration other methods that invoke _compute_parameter_bounds() and rely on the exactness of the bounds returned. One such method is _is_coordinate_fixed(), added in #3503.

A possible resolution:

1. Allow the optional index argument to _compute_parameter_bounds() to be None or either of the following:
   • A list (with length self.dim) of 2-tuple of bool, such that for each entry of each tuple, a value of True [False] indicates that the corresponding bounding problem should be solved [skipped]. In this case, make _compute_parameter_bounds() return a list of 2-tuples such that the entries corresponding to skipped bounds are of value None.
   • A list of 2-tuples corresponding to the bounding problems that should be solved. The first entry of each tuple indicates the ordinal position of the uncertain parameter and the second entry is a 0-1 value indicating the bound (lower or upper). In this case, make _compute_parameter_bounds() return a dict mapping the tuples to the calculated bounds.
2. Make use of the modified index argument to _compute_parameter_bounds() in is_bounded(), such that only bounds for which non-finite values were reported by the FBBT method are (re-)calculated.
3. Account for changes to the logic of the index argument to_compute_parameter_bounds() throughout the rest of the PyROS codebase. In particular, the method _is_coordinate_fixed() may need to be modified to account for changes to the structure of the returned bounds.

For clarity, we should probably also rename _compute_parameter_bounds() to _compute_exact_parameter_bounds() and/or ensure that the introductory one-sentence summary in the docstring explicitly conveys that the bounds returned are exact (that is, modify the sentence to "Compute exact lower and upper bounds...").

I would probably also ensure that the docstring of _fbbt_parameter_bounds() explicitly conveys that the bounds returned by _fbbt_parameter_bounds() are, or may be, inexact.

[shermanjasonaf]

Since this method does not necessarily check that the nominal point is located in the set:

Suggested change

	True if the nominal point is within the set,
	and False otherwise.
	True if the uncertainty set is nonempty,
	and False otherwise.

[shermanjasonaf]

Consider wrapping this sentence:

Suggested change

	Validate the uncertainty set with a nonemptiness and boundedness check.
	Validate the uncertainty set with a nonemptiness
	and boundedness check.

[shermanjasonaf]

Consider (re-)wrapping these sentences (and all other added/modified docstrings in this PR) to a line length of ~72 (including the indentation spaces). I generally wrap docstrings to that length to avoid long lines and since black does not wrap docstrings.

[shermanjasonaf]

This check is not (necessarily) consistent with the method docstring ("...full column rank of the LHS matrix..."). Was your final decision to check that the LHS matrix is full column rank, or that there is no column of zeros? Either the docstring or this check should be modified accordingly. If this check is to be kept as-is, then the first assignment can be simplified to:

        cols_with_all_zeros = np.nonzero(np.all(lhs_coeffs_arr == 0, axis=0))[0]

If you intend to check that the matrix is full column rank, then you can adopt the check used in FactorModelSet.validate().

[shermanjasonaf]

For this and other UncertaintySet subclass validate() method docstrings, the phrase "finiteness ... checks" should be modified for clarity. (E.g., of what is the "finiteness" being checked?) I would probably change this particular sentence to "If self.bounds contains invalid (e.g., infinite) values."

[shermanjasonaf]

Echoing the comments on the docstring of BoxSet.validate():

Suggested change

	If any uncertainty set attributes are not valid.
	If finiteness, positive deviation, or gamma checks fail.
	If any uncertainty set attributes are not valid,
	(e.g., numeric values are infinite,
	``self.positive_deviation`` has negative values,
	or ``self.gamma`` is out of range).

I would modify the other subclass-specific validate() docstrings similarly.