fix: use first line from shapely and add all dip/thickness estimates #246
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts InterpolatedStructure thickness debug outputs to use the first geometry returned by shapely.shortest_line(...) consistently, and to record all dip estimates (rather than a single aggregated entry) alongside their corresponding shortest-line geometries.
Changes:
- Use
short_line[0]consistently for length filtering and proximity queries (dwithin/distance fallback). - Record one line per dip sample by extending
_lines/_dipsinstead of appending a single entry. - Minor whitespace-only change near the
self.linesGeoDataFrame creation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # find the indices of the points that are within 5% of the length of the shortest line | ||
| try: | ||
| # GEOS 3.10.0+ | ||
| indices = shapely.dwithin(short_line, interp_points, line_length * 0.25) | ||
| indices = shapely.dwithin(short_line[0], interp_points, line_length * 0.25) | ||
| except UnsupportedGEOSVersionError: |
There was a problem hiding this comment.
The comment says points are selected within 5% of the shortest-line length, but the threshold used is line_length * 0.25 (25%). Please update the comment or adjust the factor so they match.
| indices = shapely.dwithin(short_line, interp_points, line_length * 0.25) | ||
| indices = shapely.dwithin(short_line[0], interp_points, line_length * 0.25) | ||
| except UnsupportedGEOSVersionError: | ||
| indices= numpy.array([shapely.distance(short_line[0],point)<= (line_length * 0.25) for point in interp_points]) |
There was a problem hiding this comment.
This fallback branch is not formatted according to the repo’s Black configuration (e.g., missing spaces around = and after commas). Please reformat this line to match the rest of the codebase.
| indices= numpy.array([shapely.distance(short_line[0],point)<= (line_length * 0.25) for point in interp_points]) | |
| indices = numpy.array([shapely.distance(short_line[0], point) <= (line_length * 0.25) for point in interp_points]) |
| else: | ||
| self.location_tracking = geopandas.GeoDataFrame(columns=['p1_x', 'p1_y', 'p1_z', 'p2_x', 'p2_y', 'p2_z', 'thickness', 'unit', 'geometry'], crs=basal_contacts.crs) | ||
| # Create GeoDataFrame for lines | ||
|
|
There was a problem hiding this comment.
There appears to be trailing whitespace on this blank line. Please remove the extra spaces (or delete the blank line) to avoid lint/format churn.
| if len(_dip) > 0: | ||
| _lines.extend([short_line[0]]*len(_dip)) | ||
| _dips.extend(_dip) |
There was a problem hiding this comment.
This change alters the shape/semantics of the debug output (self.lines) by emitting one row per dip sample (_lines.extend(...); _dips.extend(...)). There are existing tests for InterpolatedStructure.compute, but none assert the contents/lengths of thickness_calculator.lines; please add a regression test to cover the new behavior (e.g., that len(lines) == len(lines['dip']) and multiple dip samples produce multiple line rows).
|
@rabii-chaarani can you approve this before the workshop? we'll need to release a new version. Thanks! |
No description provided.