-
Notifications
You must be signed in to change notification settings - Fork 123
Rename ertbox -> grid_geometry #12202
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12202 +/- ##
==========================================
- Coverage 90.58% 90.54% -0.04%
==========================================
Files 435 436 +1
Lines 29933 30007 +74
==========================================
+ Hits 27114 27171 +57
- Misses 2819 2836 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/ert/storage/migration/to16.py
Outdated
| new_configs = {} | ||
| for param_config in parameters_json.values(): | ||
| if param_config["type"] == "field": | ||
| param_config["grid_dimensions"] = param_config.pop("ertbox_params") |
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.
I guess this should be param_config.pop("ertbox_params", None)?
xjules
left a comment
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.
Just had a small comment in the migration but otherwise good.
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
|
A comment: I am not sure renaming ertbox to grid_dimensions is a good approach since this contains more than grid dimensions. The |
oddvarlia
left a comment
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.
I am not sure we should rename this to grid_dimensions since the class ErtboxParameters not only contains grid dimensions (nx,ny,nz) but equally important a definition of the local coordinate system for the field parameters. grid_dimensions is not a very descriptive name of the class ErtboxParameters
b125179 to
b497559
Compare
b497559 to
8fe82d0
Compare
Good point, renamed it: |
| type: Literal["field"] = "field" | ||
| dimensionality: Literal[3] = 3 | ||
| ertbox_params: ErtboxParameters | ||
| grid_geometry: GridGeometry |
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.
I like grid_geometry!
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.
GridGeometry is better. but what about all documentation. ERTBOX has been used for a long time in courses and documentation including APS and its documentation. I don't think I will change it in APS repo, at least not now, but APS and ERT does not share code. But fmutools and APS share code and ERTBOX is consistenty used there also. The repo subscript also uses the ERTBOX as name for the field parameter grids.
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.
But maybe we can still use ERTBOX as terminology, but use GridGeometry as the name of the ERT python class. My argument for this is that GridGeometry is very general, but ERTBOX is for field parameter grid to be used as field parameters in ERT.
| def field_with_grid_params(): | ||
| # Return a Field object for test purpose | ||
| # Define parameters for the grid (ertbox parameters) | ||
| # Define parameters for the grid (grid parameters) |
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.
# Define parameters for the grid (its geometry)
xjules
left a comment
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.
Good renaming!
Optional: write a simple unit test test_to22.py
Issue
Resolves #my_issue
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable