Add soil material handler to geomorphology#95
Add soil material handler to geomorphology#95cfrontin merged 21 commits intoNLRWindSystems:offshore-developmentfrom
Conversation
…shore-development
…shore-development
…shore-development
…shore-development
…shore-development
…shore-development
…shore-development
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new soil material handler for geomorphology and updates the test suite and data files accordingly. Key changes include:
- A new unit test in test_geomorphology.py to validate soil data loading.
- Addition of a new example soil data file with defined soil types.
- Updates to the geomorphology module to parse soil grids and soil type metadata.
- Minor formatting improvements in the mooring design file.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/unit/ard/geographic/test_geomorphology.py | Adds tests for loading soil data and validating grid shapes. |
| examples/data/offshore/GulfOfMaine_soil_100x99.txt | Introduces sample soil data with soil type specifications. |
| ard/offshore/mooring_design_detailed.py | Minimal formatting update (insertion of a blank line). |
| ard/geographic/geomorphology.py | Enhances the soil file parsing functionality and data storage. |
Comments suppressed due to low confidence (2)
ard/geographic/geomorphology.py:249
- [nitpick] The variable 'sth' is ambiguous; consider renaming it to 'soil_type_headers' to make the code easier to understand.
sth = [] # soil type headers
ard/geographic/geomorphology.py:307
- Relying on an exact whitespace formatting in header comparisons can be brittle; consider normalizing the header (e.g., by splitting and comparing tokens) to improve robustness.
assert line.startswith("Class \tGamma \tSu0 \tk\talpha\tphi\tUCS\tEm")
jaredthomas68
left a comment
jaredthomas68
left a comment
There was a problem hiding this comment.
Looks good to me for the most part. Mostly just need to use warnings and errors instead of assertions.
ard/geographic/geomorphology.py
Outdated
| for idx_line, line in enumerate(f_soil.readlines()): | ||
|
|
||
| if idx_line == 0: # moorpy header line must be first | ||
| assert line.startswith("--- MoorPy Soil Input File ---") |
There was a problem hiding this comment.
I don't think it is considered good practice to use test-style assertions for error checking. We should change to explicit errors and warnings.
There was a problem hiding this comment.
Same for all instances of assert in this file
There was a problem hiding this comment.
yeah good point, will do!
There was a problem hiding this comment.
I bet co pilot could prototype the swap for your pretty easily
| assert np.all(self.bathymetry.x_material_data.shape == np.array([100, 99])) | ||
| assert np.all(self.bathymetry.y_material_data.shape == np.array([100, 99])) | ||
|
|
||
| # make sure the data matches the some properties of the original data |
jaredthomas68
left a comment
jaredthomas68
left a comment
There was a problem hiding this comment.
Looks good, a few non-blocking suggestions.
| assert len(soil_row_tgt) == nGridX # verify length | ||
| if len(soil_row_tgt) != nGridX: | ||
| raise IOError( | ||
| "Number of soil data entries does not match nGridX" |
There was a problem hiding this comment.
should this be "rows" instead of "entries"?
There was a problem hiding this comment.
well it's entries in a given row here, so it could be columns?
| idx_y += 1 # increment the y indexer | ||
| assert idx_y == nGridY # verify that all y coordinates were read | ||
| if idx_y != nGridY: | ||
| raise IOError("Number of y coordinates read does not match nGridY") |
There was a problem hiding this comment.
should we also have this y check in soil reader? I think it only checks x
There was a problem hiding this comment.
this check in particular is just to ensure that the number of lines read is correct, but there probably should also be a check that the number of elements in a line is also correct
cfrontin
merged commit ef8c5a9
into
NLRWindSystems:offshore-development
2 participants
No description provided.