-
Notifications
You must be signed in to change notification settings - Fork 107
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
Delete get_ensemble_by_name from LocalStorage
#8623
Conversation
0ec3332
to
fbc1e61
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8623 +/- ##
=======================================
Coverage 90.84% 90.85%
=======================================
Files 339 339
Lines 20897 20894 -3
=======================================
- Hits 18984 18983 -1
+ Misses 1913 1911 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Are experiment names unique? |
Yes they are. One reason why want this is that it makes it easier to read plots since we use |
@@ -314,6 +314,7 @@ def migrate_case(storage: Storage, path: Path, stack: ExitStack) -> None: | |||
parameters=parameter_configs, | |||
responses=response_configs, | |||
observations=ert_config.observations, | |||
name="migrate-case", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be made unique or is it ok for cases to be migrated (from blockfs in this case so maybe no that many) w/ the same experiment name? Logging statement above is
logger.info(f"Migrating case '{path.name}'")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point, but this not change behavior as the current code does this when no name is specified:
name = datetime.today().strftime("%Y-%m-%d")
Will leave as is for now. OK?
I could not find any logic in ERT that enforces the experiment name to be unique, but it might be that I did not look hard enough. If the experiment name is unique and this is checked when a new experiment is created then to make Everest work with these changes we would need to update:
experiment = storage.create_experiment(
parameters=self.ert_config.ensemble_config.parameter_configuration,
responses=self.ert_config.ensemble_config.response_configuration,
) to experiment = storage.create_experiment(
parameters=self.ert_config.ensemble_config.parameter_configuration,
responses=self.ert_config.ensemble_config.response_configuration,
name=f"experiment_{case_name}"
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend double checking wrt uniqueness of experiment name in some edge cases, otherwise LGTM
You are right I think, the validation is only in the front-end. @larsevj is working on making sure it's not possible to create experiments with non-unique names I think. |
Ensemble names are only unique within a single experiment.
7c9e7c2
to
17fe3b4
Compare
Ensemble names are only unique within a single experiment.
Breaking Change
semeio
to fail, so this PR will need to be merged: Dont use get_ensemble_by_name from LocalStorage semeio#640