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

Reimplement objective scaling #9985

Merged
merged 4 commits into from
Feb 7, 2025
Merged

Conversation

verveerpj
Copy link
Contributor

Issue
This PR reimplements objective scaling using the new approach of ropt to transform variables and objectives between the model and optimization domains.

Approach
There are three commits:

  1. A small commit that uses an improvement in ropt to simplify the storage code.
  2. A refactoring of the forward model evaluator in the everest run model to introduce a clearer boundary between ropt and everest code.
  3. The new implementation of objective scaling.

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@verveerpj verveerpj added release-notes:skip If there should be no mention of this in release notes everest labels Feb 5, 2025
@verveerpj verveerpj self-assigned this Feb 5, 2025
@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch from a6a29ce to 00285ba Compare February 5, 2025 19:11
Copy link

codspeed-hq bot commented Feb 5, 2025

CodSpeed Performance Report

Merging #9985 will not alter performance

Comparing reimplement-objective-scaling (7f42b79) with main (34f35d0)

Summary

✅ 25 untouched benchmarks

@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch 5 times, most recently from 5a23e90 to a3d335e Compare February 6, 2025 12:39
@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch 2 times, most recently from 4a2abee to 871533a Compare February 6, 2025 14:55
@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch from 871533a to 0aad2c2 Compare February 7, 2025 11:05
) -> EvaluatorResult:
realizations = [
self._everest_config.model.realizations[evaluator_context.realizations[idx]]
for idx in range(control_values.shape[0])
Copy link
Contributor

@yngve-sk yngve-sk Feb 7, 2025

Choose a reason for hiding this comment

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

Could we pull out range(control_values.shape[0]) to a separate variable simulation_ids here for readability? Really it is a list of all ert realizations / sim_ids, I think pulling it out would clarify that too. Similarly (even though there is just one instance) within _run_forward_model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

# implicitly given by there position in the evaluated_control_indices
# list. We store for each control vector that id, or -1 if it was not
# evaluated:
sim_ids = np.fromiter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it is more understandable I think, and could be revisited a few more times. In the long run we could maybe get rid of the -1 as evaluation id, maybe just use the ERT realization instead of it, since all evaluations are ultimately stored in ERT storage as a realization. This is future work either way

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 are going to re-evaluate these ID's anyway. These ID's, and the batch-ID are passed, so you can map a result produced by ropt to a runpath on disk. So, if you have the batch and the model realizations, you look for the folder with the right simulation_# name, where # is the simulation ID. That is a bit primitive and cumbersome, we should think about improving this, and indeed ultimately there is some mapping to a location in ERT storage.

@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch from 0aad2c2 to 9229e37 Compare February 7, 2025 11:49
@verveerpj verveerpj force-pushed the reimplement-objective-scaling branch from 9229e37 to 7f42b79 Compare February 7, 2025 11:50
Copy link
Contributor

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

LGTM

@verveerpj verveerpj merged commit 856cc02 into main Feb 7, 2025
26 checks passed
@verveerpj verveerpj deleted the reimplement-objective-scaling branch February 7, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
everest release-notes:skip If there should be no mention of this in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants