Skip to content

Add ignore_nan option to solver and implement validation skipping for…#1

Open
MGPowerlytics wants to merge 1 commit intomasterfrom
mgversion
Open

Add ignore_nan option to solver and implement validation skipping for…#1
MGPowerlytics wants to merge 1 commit intomasterfrom
mgversion

Conversation

@MGPowerlytics
Copy link
Owner

… NaN/Inf

Description

Please include a short summary of the change.
Issue link (if applicable):

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment on lines +69 to +72
try:
yield
finally:
_ignore_nan_scope_active = prev_state
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dpp_scope context manager does not use a try/finally block to restore the previous state, but ignore_nan_scope does. For consistency and robustness, both should use the same pattern. The try/finally pattern in ignore_nan_scope is better as it ensures the state is restored even if an exception occurs.

Copilot uses AI. Check for mistakes.
Comment on lines +1223 to +1247
if verbose:
print(_NUM_SOLVER_STR)
s.LOGGER.info(
'Invoking solver %s to obtain a solution.',
solving_chain.reductions[-1].name())
start = time.time()
solver_verbose = kwargs.pop('solver_verbose', verbose)
if solver_verbose and (not verbose):
print(_NUM_SOLVER_STR)
if verbose and bibtex:
print(_CITATION_STR)

# Cite CVXPY papers.
print(CITATION_DICT["CVXPY"])

# Cite problem grammar.
if self.is_dcp():
print(CITATION_DICT["DCP"])
if gp:
print(CITATION_DICT["DGP"])

# Cite solver.
print(solving_chain.reductions[-1].cite(data))
solution = solving_chain.solve_via_data(
self, data, warm_start, solver_verbose, kwargs)
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solve_via_data call should be outside the ignore_nan_scope context manager. Currently, the scope exits at line 1248, which means the actual solving happens inside the scope. However, the validation that the scope is meant to bypass only happens during get_problem_data (specifically in apply_parameters). Keeping the solver call inside the scope unnecessarily extends its lifetime and could mask validation issues in other parts of the solving process.

Suggested change
if verbose:
print(_NUM_SOLVER_STR)
s.LOGGER.info(
'Invoking solver %s to obtain a solution.',
solving_chain.reductions[-1].name())
start = time.time()
solver_verbose = kwargs.pop('solver_verbose', verbose)
if solver_verbose and (not verbose):
print(_NUM_SOLVER_STR)
if verbose and bibtex:
print(_CITATION_STR)
# Cite CVXPY papers.
print(CITATION_DICT["CVXPY"])
# Cite problem grammar.
if self.is_dcp():
print(CITATION_DICT["DCP"])
if gp:
print(CITATION_DICT["DGP"])
# Cite solver.
print(solving_chain.reductions[-1].cite(data))
solution = solving_chain.solve_via_data(
self, data, warm_start, solver_verbose, kwargs)
if verbose:
print(_NUM_SOLVER_STR)
s.LOGGER.info(
'Invoking solver %s to obtain a solution.',
solving_chain.reductions[-1].name())
start = time.time()
solver_verbose = kwargs.pop('solver_verbose', verbose)
if solver_verbose and (not verbose):
print(_NUM_SOLVER_STR)
if verbose and bibtex:
print(_CITATION_STR)
# Cite CVXPY papers.
print(CITATION_DICT["CVXPY"])
# Cite problem grammar.
if self.is_dcp():
print(CITATION_DICT["DCP"])
if gp:
print(CITATION_DICT["DGP"])
# Cite solver.
print(solving_chain.reductions[-1].cite(data))
solution = solving_chain.solve_via_data(
self, data, warm_start, solver_verbose, kwargs)

Copilot uses AI. Check for mistakes.
Comment on lines +839 to +841
# Set ignore_nan flag on param_prog if specified in solver_opts
if solver_opts and solver_opts.get('ignore_nan', False):
self._cache.param_prog.ignore_nan = True
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a redundancy in how ignore_nan is handled. The code sets self._cache.param_prog.ignore_nan both in lines 800-803 and 839-841. Additionally, the ignore_nan_scope context manager is being used alongside these flag settings, creating two overlapping mechanisms. This dual approach is confusing - either use the context manager consistently or use the flag consistently, but mixing both adds unnecessary complexity.

Suggested change
# Set ignore_nan flag on param_prog if specified in solver_opts
if solver_opts and solver_opts.get('ignore_nan', False):
self._cache.param_prog.ignore_nan = True

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants