Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating optimize to handle multiple risk constraints #596

Merged
merged 34 commits into from
Aug 27, 2024

Conversation

anirban-chaudhuri
Copy link
Contributor

@anirban-chaudhuri anirban-chaudhuri commented Jul 31, 2024

  • Adding support for multiple risk constraints: requires risk_bound, alpha, and qoi to be List.

@anirban-chaudhuri anirban-chaudhuri added enhancement New feature or request integration Tasks for integration with TA4 labels Jul 31, 2024
@anirban-chaudhuri anirban-chaudhuri self-assigned this Jul 31, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@anirban-chaudhuri anirban-chaudhuri linked an issue Jul 31, 2024 that may be closed by this pull request
@anirban-chaudhuri anirban-chaudhuri changed the title Updating optimize to handle multiple risk constraints Updating optimize to handle multiple risk constraints Jul 31, 2024
@anirban-chaudhuri anirban-chaudhuri added the WIP PR submitter still making changes, not ready for review label Jul 31, 2024
@anirban-chaudhuri anirban-chaudhuri added awaiting review PR submitter awaiting code review from reviewer and removed WIP PR submitter still making changes, not ready for review labels Aug 9, 2024
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

Looks good overall! That said, I think it would be helpful to refine the types and avoid squeeze where possible. See individual comments for details.

pyciemss/interfaces.py Outdated Show resolved Hide resolved
pyciemss/interfaces.py Outdated Show resolved Hide resolved
pyciemss/interfaces.py Show resolved Hide resolved
pyciemss/ouu/ouu.py Outdated Show resolved Hide resolved
pyciemss/ouu/qoi.py Outdated Show resolved Hide resolved
end_time: float,
logging_step_size: float,
*,
start_time: float = 0.0,
risk_measure: Callable = lambda z: alpha_superquantile(z, alpha=0.95),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Could we rename this argument risk_measures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but I wonder what would be the downstream effects of changing the name here. I am guessing it should be doable...

pyciemss/ouu/ouu.py Show resolved Hide resolved
docs/source/interfaces.ipynb Outdated Show resolved Hide resolved
docs/source/optimize_interface.ipynb Outdated Show resolved Hide resolved
@SamWitty SamWitty added awaiting response PR reviewer awaiting response from submitter and removed awaiting review PR submitter awaiting code review from reviewer labels Aug 14, 2024
@anirban-chaudhuri anirban-chaudhuri added awaiting review PR submitter awaiting code review from reviewer and removed awaiting response PR reviewer awaiting response from submitter labels Aug 27, 2024
@anirban-chaudhuri
Copy link
Contributor Author

Looks good overall! That said, I think it would be helpful to refine the types and avoid squeeze where possible. See individual comments for details.

All the squeeze have now been removed from optimize, but there is still one in results processing which should be a separate PR.

Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

Looks great!

@SamWitty SamWitty merged commit 5401b75 into main Aug 27, 2024
5 checks passed
@SamWitty SamWitty deleted the ac-multiple-constraints branch August 27, 2024 16:59
@liunelson
Copy link
Contributor

@anirban-chaudhuri
Could you confirm that I'm using your new interface correctly?

Suppose I want the following "success criteria" or "constraints":

  • Ensure S is below 1 at all timepoints (risk level 95%)
  • Ensure S is above 1 at the last timepoint (risk level 95%)
    image
observed_params = [["S_state"], ["S_state"]]
risk_bound = [1.0, -2.0]
alpha = [0.95, 0.95]
qoi = [
    lambda y: obs_max_qoi(y, observed_params[0]),
    lambda y: -obs_nday_average_qoi(y, observed_params[1])
]

Is the minus sign on the risk bound and qoi function the right way to specify an "above threshold" constraint?

@liunelson
Copy link
Contributor

Also, how is 0.4 in scaling_factor_obj[1] chosen in cell 25?
https://github.com/liunelson/pyciemss/blob/nl-test/docs/source/optimize_interface.ipynb

I'd have expected it to be just 1.0 / (bounds_interventions[1][1] - bounds_interventions[0][1])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR submitter awaiting code review from reviewer enhancement New feature or request integration Tasks for integration with TA4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for multiple constraints in Optimize
3 participants