GEOPY-2731: Replace InputFile for BaseUIJson class - #134
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates curve-apps away from geoh5py.ui_json.InputFile toward the newer geoh5py.ui_json.UIJson / geoapps_utils.base.Options flow, updating both runtime drivers/apps and the test suite to build and run from UIJson-backed parameter models.
Changes:
- Updated tests and drivers to use
UIJson.read()/write()andOptions.build(...)instead ofInputFile.read_ui_json()/write_ui_json(). - Refactored Peak Finder parameters from the legacy BaseParams pattern to an
Optionsmodel and adjusted driver/app integration accordingly. - Tweaked UI JSON assets (e.g., Peak Finder group
dataTypeentries) and contour fixed-contour serialization behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/trend_line_run_test.py | Updates Trend Lines tests to use UIJson and new build(...) calling convention. |
| tests/peak_finder/peak_finder_test.py | Updates Peak Finder tests to build params directly from objects/kwargs rather than UIJson string payloads. |
| tests/parameters_test.py | Switches validation assertions to UIJson access patterns and updated contour fixed-contour serialization expectations. |
| tests/edge_detection_run_test.py | Updates Edge Detection tests to use UIJson and Options.build(...). |
| tests/contour_test.py | Adjusts ContourParameters.build(...) invocation to match new API. |
| curve_apps/trend_lines/driver.py | Removes InputFile constructor support and imports in favor of Options. |
| curve_apps/peak_finder/peak_finder.geoh5 | Removes the (LFS pointer) .geoh5 asset from the repo. |
| curve_apps/peak_finder/params.py | Refactors Peak Finder params into an Options model. |
| curve_apps/peak_finder/layout.py | Updates a layout html.Div to explicit children= with flex styling. |
| curve_apps/peak_finder/driver.py | Updates Peak Finder driver to use Options patterns and adds get_line_field helper. |
| curve_apps/peak_finder/base_dash.py | Migrates dash base application from BaseParams to Options/ui_json.flatten(). |
| curve_apps/peak_finder/application.py | Switches CLI entrypoint to UIJson.read and PeakFinderParams.build(...); simplifies trigger handling. |
| curve_apps/edges/driver.py | Removes InputFile constructor support and imports in favor of Options. |
| curve_apps/driver.py | Removes InputFile constructor handling from BaseCurveDriver. |
| curve_apps/contours/options.py | Adds serializer for fixed contours to persist as comma-separated string in ui.json output. |
| curve_apps/contours/driver.py | Removes InputFile constructor support and imports in favor of Options. |
| curve_apps-assets/uijson/peak_finder.ui.json | Adds dataType: Float metadata for Peak Finder data groups. |
| curve_apps-assets/uijson/contours.ui.json | Removes one meshType entry from contour ui.json. |
Suppressed comments (1)
curve_apps/peak_finder/application.py:1983
- In the
__main__launch-dash path,_ = peak_parameters.surveyno longer "triggers computation" becausePeakFinderParams.surveyis now just a plain field (the computed in-memory survey copy lives onPeakFinder.survey). As written, this line is a no-op and the comment is misleading.
if peak_parameters.launch_dash:
with peak_parameters.geoh5.open(mode="r"):
_ = peak_parameters.survey # Trigger computation
logger.info("Loaded. Launching peak finder app . . .")
PeakFinder(peak_parameters).run()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #134 +/- ##
===========================================
- Coverage 67.97% 64.57% -3.41%
===========================================
Files 20 20
Lines 2364 2120 -244
Branches 325 323 -2
===========================================
- Hits 1607 1369 -238
+ Misses 654 651 -3
+ Partials 103 100 -3
🚀 New features to boost your workflow:
|
gmcga
left a comment
There was a problem hiding this comment.
Noticed a couple of small potential issues, other than that seems good!
| @@ -436,16 +423,34 @@ def run(self): # pylint: disable=too-many-locals, too-many-branches, too-many-s | |||
|
|
|||
| self.update_monitoring_directory(out_group) | |||
There was a problem hiding this comment.
run creates a local out_group but neither stores or returns it, which will probably cause problems with the new Driver.start as it will receive result = None
There was a problem hiding this comment.
Technically no issue because the driver.out_group is not None. But agree that update_monitoring_directory(out_group) is already in the start
GEOPY-2731 - Replace InputFile for BaseUIJson class