GEOPY-3013: Support Collar dip and Collar azimuth drillhole attributes - #962
Conversation
github-actions
Bot
changed the title
There was a problem hiding this comment.
🟡 Not ready to approve
The current surveys=None defaulting logic can produce incorrect dips (e.g., collar_dip=0.0) and can unintentionally overwrite an explicitly provided end_of_hole.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds support for defining a drillhole’s initial orientation via collar azimuth and collar dip, and uses those values to drive desurveying when explicit survey data isn’t provided.
Changes:
- Add
collar_azimuthandcollar_dipattributes toDrillholeand related attribute key maps. - Update
Drillhole.surveysdefaulting behavior to derive a single survey point from collar azimuth/dip when surveys are omitted. - Add a regression test validating desurvey results for collar azimuth/dip without surveys.
File summaries
| File | Description |
|---|---|
| tests/objects/drillhole_data_test.py | Adds a test for collar azimuth/dip desurvey behavior; standardizes temp workspace filename creation. |
| geoh5py/shared/utils.py | Extends key mapping to recognize “Collar azimuth” and “Collar dip” attributes. |
| geoh5py/objects/drillhole.py | Introduces collar azimuth/dip attributes and uses them to default surveys when surveys are not provided. |
Review details
Comments suppressed due to low confidence (1)
geoh5py/objects/drillhole.py:332
- When
surveys=None, the default dip/azimuth usesor, which treats validcollar_dip=0.0as falsy and incorrectly falls back to-90.0. Also, the setter always overwritesend_of_holefrom the (default) surveys, which clobbers any explicitend_of_holepassed during initialization.
def surveys(self, array: np.ndarray | list | tuple | None):
if array is None:
array = np.c_[0, self._collar_azimuth or 0.0, self._collar_dip or -90.0]
if not isinstance(array, (np.ndarray, list, tuple)):
raise TypeError(
"Input 'surveys' must be of type 'numpy.ndarray' or 'list'."
)
self._surveys = self.format_survey_values(array)
self.end_of_hole = float(self._surveys["Depth"][-1])
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated drillhole survey handling has correctness issues (defaulting via or and structured-array formatting) and a key test assertion was commented out, reducing regression protection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (3)
geoh5py/objects/drillhole.py:383
format_survey_valuescurrently transposes structured/recarray inputs and, when dropping the optionalInfofield, slices records (values[:-1]) instead of fields. This will break callers that pass a structured surveys array (e.g., tests that use a 4-field dtype withInfo) and can raise atnp.rec.fromarrays.
# Already a recarray with proper type
if values.dtype.names == dtype.names:
return values
if np.issubdtype(values.dtype, np.number):
n_fields = values.shape[1]
else:
n_fields = len(values.dtype.names)
if n_fields not in [3, 4]:
raise ValueError("'surveys' requires an ndarray of shape (*, 3) or (*, 4)")
values = values.T.tolist()
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]), dtype=dtype[-1])]
elif n_fields == 4 and len(dtype) == 3:
values = values[:-1]
array_values = np.rec.fromarrays(values, dtype=dtype)
geoh5py/objects/drillhole.py:330
- Using
orto provide defaults forcollar_azimuth/collar_diptreats valid0.0values as missing (because0.0is falsy), which can silently change the default survey direction.
This issue also appears on line 364 of the same file.
def surveys(self, array: np.ndarray | list | tuple | None):
if array is None:
array = np.c_[0, self._collar_azimuth or 0.0, self._collar_dip or -90.0]
tests/objects/drillhole_v4_0_test.py:1156
- The test is explicitly modifying surveys to include an
Infofield, but the assertion that verifiesInfois present is commented out. This weakens the test and can allow regressions where the extra field is dropped during concatenation/saving.
assert len(dh.parent.data["Surveys"]) == 35
# assert "Info" in dh.parent.data["Surveys"].dtype.names
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #962 +/- ##
===========================================
+ Coverage 91.16% 91.30% +0.14%
===========================================
Files 129 129
Lines 10873 10877 +4
Branches 1985 1982 -3
===========================================
+ Hits 9912 9931 +19
+ Misses 504 497 -7
+ Partials 457 449 -8
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Not ready to approve
The updated drillhole survey initialization/loading and survey reformatting contain confirmed logic issues that can break persisted survey handling and structured array conversion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (6)
geoh5py/objects/drillhole.py:120
__init__now unconditionally callsself.surveys = surveys. WhensurveysisNone, this eagerly creates a default survey and prevents any lazy loading of an existing on-file surveys array (previously fetched on demand), which can cause reopened drillholes to ignore the surveys stored in the workspace file.
self.collar = collar
self.cost = cost
self.default_collocation_distance = default_collocation_distance
self.end_of_hole = end_of_hole
self.planning = planning
self.surveys = surveys
geoh5py/objects/drillhole.py:317
- The
surveysgetter assumes_surveysis always populated in-memory. For entities loaded from disk/concatenation, surveys were previously lazily fetched viaworkspace.fetch_array_attribute(..., "surveys"); without that, reopened drillholes can return the default survey (or fail) instead of the persisted surveys.
def surveys(self) -> np.ndarray:
"""
Coordinates of the surveys.
"""
surveys = np.vstack(
geoh5py/objects/drillhole.py:106
_surveysis no longer initialized on the instance. If__init__stops calling thesurveyssetter (or if an on-file entity is constructed without surveys),surveysaccess will raiseAttributeErroror behave inconsistently. Initialize_surveystoNonelike the other cached arrays.
This issue also appears in the following locations of the same file:
- line 115
- line 313
self._depths: FloatData | None = None
self._trace: np.ndarray | None = None
self._trace_depth: np.ndarray | None = None
self._locations = None
self._intervals: dict | None = None
geoh5py/objects/drillhole.py:370
format_survey_valueswill crash withTypeError: object of type 'NoneType' has no len()if a non-numeric, non-structured ndarray is passed (e.g., dtype '<U' from mixed inputs), becausevalues.dtype.namesisNone. This should raise a clearTypeErrorinstead.
if np.issubdtype(values.dtype, np.number):
n_fields = values.shape[1]
else:
n_fields = len(values.dtype.names)
dtype = np.dtype(SURVEYS_FIELDS[:n_fields])
if values.dtype.descr != dtype.descr:
raise TypeError(f"The type of survey array must be numeric or {dtype}")
geoh5py/objects/drillhole.py:385
- When converting a 4-column input to a 3-field dtype, the code currently does
values = [val[:-1] for val in values], which removes the last row from each column array instead of dropping the 4th column. This produces misaligned survey data (length N-1) and will breaknp.rec.fromarrays.
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]), dtype=dtype[-1])]
elif n_fields == 4 and len(dtype) == 3:
values = [val[:-1] for val in values]
tests/objects/drillhole_v4_0_test.py:1155
- This test currently comments out the assertion that the concatenated
Surveysdtype contains theInfofield. Leaving a disabled assertion makes it easy for regressions in survey-info handling to go unnoticed; update the implementation to satisfy the assertion and keep the coverage.
assert len(dh.parent.data["Surveys"]) == 35
# assert "Info" in dh.parent.data["Surveys"].dtype.names
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import numpy as np | ||
| import pytest | ||
| from h5py import special_dtype | ||
| from h5py import special_dtype, string_dtype |
There was a problem hiding this comment.
🟡 Not ready to approve
Default survey construction and empty Info handling have correctness risks (e.g., collar_dip=0.0 treated as unset and bytes used for a vlen=str field) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (5)
tests/objects/drillhole_v4_0_test.py:1136
- The
Infofield usesspecial_dtype(vlen=str), which expects Pythonstrvalues; appendingb""(bytes) can lead to type issues when building/writing the structured array.
for val in surveys:
values.append((*val, b""))
geoh5py/objects/drillhole.py:384
- When adding an empty
Infocolumn, the code currently fills it withb""(bytes). Forspecial_dtype(vlen=str)(and other string dtypes), values should be Pythonstr(e.g.,"") to avoid type mismatches when constructing/writing the structured array.
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]))]
elif n_fields == 4 and len(dtype) == 3:
dtype = np.dtype(SURVEYS_FIELDS, align=True)
tests/objects/drillhole_v4_0_test.py:1131
SURVEYS_FIELDSis imported but not used here, and the survey dtype is duplicated inline. Using the shared constant keeps tests aligned with the production survey dtype and avoids an unused import.
new_dtype = np.dtype(
[
("Depth", "<f4"),
("Azimuth", "<f4"),
("Dip", "<f4"),
("Info", special_dtype(vlen=str)),
],
align=True,
)
tests/objects/drillhole_data_test.py:30
special_dtypeis imported but not used in this test module, which can trigger unused-import checks in some lint configurations.
from h5py import special_dtype, string_dtype
geoh5py/objects/drillhole.py:331
- Using
orto apply defaults treats valid0.0values as falsy. In particular,collar_dip=0.0will be replaced by-90.0whensurveysis None, producing an incorrect default survey.
if array is None:
array = np.c_[0, self._collar_azimuth or 0.0, self._collar_dip or -90.0]
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated survey defaulting/formatting logic has edge-case bugs (e.g., dip=0.0 handling and dtype/shape handling) that can lead to incorrect surveys or runtime errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (3)
geoh5py/objects/drillhole.py:366
format_survey_valuesassumes a 2D array when it computesvalues.shape[1]. If callers pass a 1Dnp.ndarray(e.g.,np.array([depth, az, dip])), this raisesIndexErrorinstead of a clear validation error.
Normalize non-structured ndarrays to at least 2D before accessing shape[1].
if isinstance(values, (list, tuple)):
values = np.array(values, ndmin=2)
if np.issubdtype(values.dtype, np.number) or not values.dtype.names:
n_fields = values.shape[1]
else:
geoh5py/objects/drillhole.py:385
- The logic handling mismatches between
n_fieldsand the requesteddtypeis backwards:
- If the input has 4 columns but
dtypehas 3 fields, the current code forcesdtypeto 4 fields, which can break callers that explicitly requested a 3-field survey dtype (e.g., concatenation where the stored Surveys dataset is 3-field). - When adding an empty
Infocolumn, the filler array is created fromb""bytes without matching the vlen-str dtype.
Prefer adapting the values to the requested dtype: drop the 4th column when dtype has 3 fields, and add an empty string column when dtype has 4 fields.
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]))]
elif n_fields == 4 and len(dtype) == 3:
dtype = np.dtype(SURVEYS_FIELDS, align=True)
geoh5py/objects/drillhole.py:331
- When surveys are not provided, the default survey uses
self._collar_dip or -90.0. This treats a valid dip of0.0(horizontal) as falsy and incorrectly replaces it with-90.0.
Use an explicit is None check so 0.0 is preserved.
This issue also appears in the following locations of the same file:
- line 361
- line 381
if array is None:
array = np.c_[0, self._collar_azimuth or 0.0, self._collar_dip or -90.0]
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
gmcga
left a comment
gmcga
left a comment
There was a problem hiding this comment.
See comments. I also think it'd be good to try and increase the test coverage as there's a fair amount missing.
There was a problem hiding this comment.
🟡 Not ready to approve
Survey formatting in concatenated drillholes can produce a dtype that doesn’t match concatenated storage (and Info padding uses bytes), risking runtime errors and inconsistent round-tripping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
geoh5py/shared/concatenation/drillhole.py:405
- In concatenated drillholes, this override no longer normalizes incoming surveys to the concatenated 'Surveys' dtype. If the concatenated dataset has only (Depth, Azimuth, Dip) but callers provide an Info column, super().format_survey_values will currently return a 4-field recarray, which won't match the concatenated storage dtype and can break updates.
# Check if 'Info' is in the survey array
if "Surveys" in self.concatenator.data:
dtype = self.concatenator.data["Surveys"].dtype
return super().format_survey_values(values, dtype=dtype)
geoh5py/objects/drillhole.py:386
- Padding a missing 'Info' field with a bytes array (b"") can lead to mixed bytes/str values for an h5py vlen=str dtype and inconsistent round-tripping. Prefer padding with empty Python strings and the expected Info field dtype.
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]))]
geoh5py/objects/drillhole.py:374
- As written, purely numeric ndarrays with shape (, 4) are accepted and will be coerced into a dtype that includes the string 'Info' field, which is ambiguous and likely unintended. Numeric survey arrays should stay constrained to shape (, 3) (Depth/Azimuth/Dip), while 4-field inputs should be provided as structured arrays (or non-numeric arrays) with a valid Info field.
if np.issubdtype(values.dtype, np.number) or not values.dtype.names:
n_fields = values.shape[1]
else:
n_fields = len(values.dtype.names)
dtype = np.dtype(SURVEYS_FIELDS[:n_fields], align=True)
if values.dtype.descr != dtype.descr:
raise TypeError(f"The type of survey array must be numeric or {dtype}")
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new survey auto-expansion to include the optional “Info” field inserts a bytes default (b"") for a vlen str dtype, which can produce incorrect string typing in stored/returned survey records.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
geoh5py/objects/drillhole.py:387
- When auto-expanding 3-column survey inputs to a 4-field dtype (adding the optional "Info" field), the default value is currently created as a bytes array (b""). The declared dtype for "Info" is
special_dtype(vlen=str), which is intended for Pythonstrobjects; writing bytes here can lead to mixed/incorrect string types in the resulting recarray and in the persisted HDF5 dataset.
values = values.T.tolist()
if n_fields == 3 and len(dtype) == 4:
values += [np.array([b""] * len(values[0]))]
elif n_fields == 4 and len(dtype) == 3:
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
GEOPY-3013 - Support Collar dip and Collar azimuth drillhole attributes