Repository navigation
Grid overrides: NonBinned & HasDuplicates - over UPDATED v1 - #633
dmitriyrepin wants to merge 9 commits into
Conversation
dmitriyrepin
commented
Sep 4, 2025
dmitriyrepin
commented
BrianMichell
left a comment
BrianMichell
left a comment
There was a problem hiding this comment.
Please be sure to run pre-commit before pushing. You can use nox to set this up as a hook to ensure that part of the CI pipeline will pass.
| return dimensions, chunksize, headers_subset | ||
| # We need to return both dimension and coordinate headers | ||
| horizontal_coordinates = template.coordinate_names | ||
| coordinate_headers = parse_headers(segy_file=segy_file, subset=horizontal_coordinates) |
There was a problem hiding this comment.
parse_headers is a very expensive operation. Please see if you can find a way to re-use the output from the call above.
| # We need to return both dimension and coordinate headers | ||
| horizontal_coordinates = template.coordinate_names | ||
| coordinate_headers = parse_headers(segy_file=segy_file, subset=horizontal_coordinates) | ||
| headers_subset = rfn.merge_arrays((dimension_headers, coordinate_headers), asrecarray=True, flatten=True) |
There was a problem hiding this comment.
This merge function is single threaded and computationally expensive. Please investigate whether there are any better approaches.
BrianMichell
left a comment
BrianMichell
left a comment
There was a problem hiding this comment.
Great work @dmitriyrepin! Thank you for resolving the regressions so quicky.
|
|
||
| return dimensions, chunksize | ||
| if grid_overrides.get("NonBinned") or grid_overrides.get("HasDuplicates"): | ||
| horiz_dims_coords_headers = rfn.append_fields( |
There was a problem hiding this comment.
Untested on scale data that would enter this branch, but resolves the regression for binned data
# Conflicts: # src/mdio/converters/segy.py # src/mdio/schemas/v1/templates/abstract_dataset_template.py # tests/integration/conftest.py # tests/integration/test_segy_import_export.py
|
The logic of this is completely wrong and only works for a Template type that doesn't usually have these problems. I am going to merge this after minor corrections is but needs a large rework. |
|
after looking at this in more detail, this breaks other functionality and needs to be reimplemented. going to close. |
3 participants
