-
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
Add experiment name #6688
Add experiment name #6688
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6688 +/- ##
==========================================
+ Coverage 83.57% 83.62% +0.05%
==========================================
Files 359 360 +1
Lines 20968 21025 +57
Branches 948 948
==========================================
+ Hits 17523 17582 +59
+ Misses 3151 3149 -2
Partials 294 294 ☔ View full report in Codecov by Sentry. |
47a9875
to
2834988
Compare
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.
LGTM, but good i @oyvindeide takes a look as well.
2834988
to
7c16890
Compare
@@ -42,6 +43,11 @@ def __init__( | |||
|
|||
self.setObjectName("ensemble_smoother_panel") | |||
|
|||
self.name_field = QLineEdit() |
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.
Even though its not consistent we try to name private class members with _. self._name_field
) | ||
|
||
experiment_id.migrate(self.path) | ||
self._add_migration_information(4, "experiment_id") |
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.
This will have to be added to the other versions as well. The reason is because we always jump straight to the latest version, and dont upgrade iteratively. It might be time to start doing that, but that is outside the scope here.
Looks like a good improvement! Just a comment about the migrations that needs to be addressed. Just remember to squash commits before merging. |
e5628cb
to
c2a9c3d
Compare
Add input field for experiment name Add migration for experiment name Current default name set through migration isn't sensible at all due to the limited metadata we have access to at migration time Rename private class member Add experiment_id migration to all versions
c2a9c3d
to
d7d9c03
Compare
Issue
Resolves #6386
Approach
Adds a text input field whose placeholder text is the simulation mode.
If no name is provided, the placeholder text is used as the experiment name.
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist