Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive "Data Converter Widget" that enables users to convert geological datasets for use within LoopStructural through both automatic and manual conversion workflows.
Changes:
- Added a new data conversion widget with automatic and manual conversion modes
- Implemented utility functions to convert GeoPandas GeoDataFrames and pandas DataFrames to QGIS vector layers
- Integrated the data conversion widget into the main Loop widget interface
- Added tests for the NTGS configuration model
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/gui/test_ntgs_configuration_model.py | Unit tests for configuration model (imports incorrect class names) |
| loopstructural/main/vectorLayerWrapper.py | New functions to convert DataFrames/GeoDataFrames to QGIS layers |
| loopstructural/gui/loop_widget.py | Integration of data conversion widget into main interface |
| loopstructural/gui/data_conversion/data_conversion_widget.py | Main widget implementation with automatic and manual conversion modes |
| loopstructural/gui/data_conversion/configuration.py | Configuration state management for NTGS data types |
| loopstructural/gui/data_conversion/init.py | Package initialization exposing DataConversionWidget |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,33 @@ | |||
| import pytest | |||
|
|
|||
| from loopstructural.gui.data_conversion.configuration import NtgsConfig, NtgsConfigurationModel | |||
There was a problem hiding this comment.
The test imports NtgsConfig and NtgsConfigurationModel, but the actual configuration module defines these classes as Config and ConfigurationState. This import will fail at runtime. The imports should be updated to match the actual class names in loopstructural/gui/data_conversion/configuration.py.
| for _, row in df.iterrows(): | ||
| feat = QgsFeature(layer.fields()) | ||
| attrs = [] | ||
| for column in df.columns: | ||
| val = row[column] | ||
| if pd.isna(val): | ||
| val = None | ||
| elif isinstance(val, np.generic): | ||
| try: | ||
| val = val.item() | ||
| except Exception: | ||
| pass | ||
| attrs.append(val) | ||
| feat.setAttributes(attrs) | ||
| features.append(feat) |
There was a problem hiding this comment.
Using iterrows() is generally inefficient for large DataFrames as it creates a new Series object for each row. For better performance with large datasets, consider using itertuples() or vectorized operations where possible.
loopstructural/gui/loop_widget.py
Outdated
| self.data_conversion_widget = DataConversionWidget( | ||
| self, | ||
| data_manager=self.data_manager, | ||
| project=self.data_manager.project if self.data_manager else None, |
There was a problem hiding this comment.
Accessing self.data_manager.project could raise an AttributeError if data_manager is not None but lacks a project attribute. Consider using getattr(self.data_manager, 'project', None) for safer attribute access.
| project=self.data_manager.project if self.data_manager else None, | |
| project=getattr(self.data_manager, "project", None) if self.data_manager else None, |
| return layer | ||
|
|
||
|
|
||
| def QgsLayerFromDataFrame(dataframe, layer_name: str = "Converted Table"): |
There was a problem hiding this comment.
The function name QgsLayerFromDataFrame uses PascalCase, which is inconsistent with existing functions in this file like qgsLayerToGeoDataFrame, qgsLayerToDataFrame, and qgsRasterToGdalDataset that use camelCase. Consider renaming to qgsLayerFromDataFrame to maintain consistency with the existing naming convention in this file.
| def QgsLayerFromDataFrame(dataframe, layer_name: str = "Converted Table"): | |
| def qgsLayerFromDataFrame(dataframe, layer_name: str = "Converted Table"): |
| try: | ||
| if temp.createFromWkt(text): | ||
| return temp | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| temp = QgsCoordinateReferenceSystem(crs_info) | ||
| if temp.isValid(): | ||
| return temp | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Ignore failures when interpreting CRS from string; fall back to default CRS. |
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| logger.debug("Failed to convert numpy scalar %r using .item(): %s", val, exc) |
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| # Fall back to the original NumPy scalar if conversion fails. | |
| logger.debug("Failed to convert NumPy scalar %r with .item(): %s", val, exc) |
|
Can you make the data converter its own dialog or dock widget? I think it is an addition to the modelling process rather than a first step as you can use the modelling without using the data conversion. Also do you have a demo dataset I can use to test it? |
|
@rabii-chaarani looks good, couple of easy things
Future idea
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if unit_name_field != 'UNITNAME' and unit_name_field in geology_gdf.columns: | ||
| geology_gdf = geology_gdf.rename(columns={unit_name_field: 'UNITNAME'}) |
There was a problem hiding this comment.
The unit name field is being renamed twice: once at line 492-494 and again at lines 540-541. This is redundant and could lead to confusion. The second rename should be removed as the first one already handles this transformation.
| if unit_name_field != 'UNITNAME' and unit_name_field in geology_gdf.columns: | |
| geology_gdf = geology_gdf.rename(columns={unit_name_field: 'UNITNAME'}) |
loopstructural/main/m2l_api.py
Outdated
| ) | ||
| sampled_contacts_gdf = qgsLayerToGeoDataFrame(sampled_contacts) | ||
| structure_gdf = qgsLayerToGeoDataFrame(structure) | ||
| structure_gdf = qgsLayerToGeoDataFrame(structure) |
There was a problem hiding this comment.
Trailing whitespace found on line 460. This should be removed to maintain code cleanliness and consistency with the codebase style.
| if self.project is not None: | ||
| try: | ||
| combo.setProject(self.project) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Some QGIS/Qt environments may not support setProject or may raise here; | |
| # failure to bind the project is non-fatal, so we intentionally ignore it. |
| try: | ||
| data = bytes(data) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| # Best-effort conversion to bytes; if this fails, fall back to using the | |
| # original data and let the subsequent fromWkb call handle it. | |
| logger.debug( | |
| "Failed to convert WKB data to bytes in _geometry_from_value", | |
| exc_info=True, | |
| ) |
| except Exception: | ||
| pass | ||
| try: | ||
| epsg = crs_info.to_epsg() | ||
| if epsg: | ||
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | ||
| except Exception: | ||
| pass | ||
| if isinstance(crs_info, str): | ||
| try: | ||
| temp = QgsCoordinateReferenceSystem(crs_info) | ||
| if temp.isValid(): | ||
| return temp | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| try: | |
| epsg = crs_info.to_epsg() | |
| if epsg: | |
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | |
| except Exception: | |
| pass | |
| if isinstance(crs_info, str): | |
| try: | |
| temp = QgsCoordinateReferenceSystem(crs_info) | |
| if temp.isValid(): | |
| return temp | |
| except Exception: | |
| pass | |
| except Exception as e: | |
| logger.debug("Failed to create CRS from WKT using createFromWkt: %r (%s)", text, e) | |
| try: | |
| epsg = crs_info.to_epsg() | |
| if epsg: | |
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | |
| except Exception as e: | |
| logger.debug("Failed to derive EPSG from CRS info %r: %s", crs_info, e) | |
| if isinstance(crs_info, str): | |
| try: | |
| temp = QgsCoordinateReferenceSystem(crs_info) | |
| if temp.isValid(): | |
| return temp | |
| except Exception as e: | |
| logger.debug("Failed to construct CRS from string %r: %s", crs_info, e) |
| except Exception: | ||
| pass | ||
| try: | ||
| epsg = crs_info.to_epsg() | ||
| if epsg: | ||
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | ||
| except Exception: | ||
| pass | ||
| if isinstance(crs_info, str): | ||
| try: | ||
| temp = QgsCoordinateReferenceSystem(crs_info) | ||
| if temp.isValid(): | ||
| return temp | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| try: | |
| epsg = crs_info.to_epsg() | |
| if epsg: | |
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | |
| except Exception: | |
| pass | |
| if isinstance(crs_info, str): | |
| try: | |
| temp = QgsCoordinateReferenceSystem(crs_info) | |
| if temp.isValid(): | |
| return temp | |
| except Exception: | |
| pass | |
| except Exception as exc: | |
| logger.debug( | |
| "Failed to create CRS from WKT using legacy createFromWkt: %s", | |
| exc, | |
| ) | |
| try: | |
| epsg = crs_info.to_epsg() | |
| if epsg: | |
| return QgsCoordinateReferenceSystem.fromEpsgId(int(epsg)) | |
| except Exception as exc: | |
| logger.debug( | |
| "Failed to obtain EPSG code from CRS info %r: %s", | |
| crs_info, | |
| exc, | |
| ) | |
| if isinstance(crs_info, str): | |
| try: | |
| temp = QgsCoordinateReferenceSystem(crs_info) | |
| if temp.isValid(): | |
| return temp | |
| except Exception as exc: | |
| logger.debug( | |
| "Failed to construct CRS from string %r: %s", | |
| crs_info, | |
| exc, | |
| ) |
| elif isinstance(val, np.generic): | ||
| try: | ||
| val = val.item() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # If conversion to a native Python scalar fails, keep the original numpy scalar. |
No description provided.