-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Iso surface #54
Iso surface #54
Conversation
# Conflicts: # geoapps/utils/utils.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice feature !
please see various suggestions / questions
(not reviewing the ipynb files)
@@ -135,9 +139,6 @@ def trigger_click(self): | |||
y, | |||
) | |||
|
|||
if self.code_out.value == "EPSG:4326": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is the flip of x and y some kind of work-around for a bug in the 3rd party?
Maybe add a comment line 131 to explain it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like they finally made the good call of exporting as long-lat. Before going from cartesian (xyz) would return (lat-lon) which basically permutes the axies. I just found that building the docs today. Looks like the reverted from a previous version: OSGeo/gdal#1546
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formerly there were two swaps: line 131 and here. Should not the swap at line 131 be removed as well then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domfournier, do you confirm the swap it line 131 must stay? I am somewhat confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. Right now it makes it "work", but I will have to drill down a bit more.
|
||
Parameters | ||
---------- | ||
xyz_in: numpy.ndarray shape(*, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (non-blocking): why not using typing in the signature?
else: | ||
vertices = np.vstack(vertices).T | ||
except RuntimeError: | ||
vertices, faces = [], [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: should log the detail of the exception?
we do not really expect one, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if vertices is empty yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case, it might be clearer to test for empty vertices
instead of letting the exception happen
Co-authored-by: Sébastien Hensgen <sebastienh@mirageoscience.com>
Co-authored-by: Sébastien Hensgen <sebastienh@mirageoscience.com>
Codecov Report
@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 20.81% 18.92% -1.89%
==========================================
Files 146 146
Lines 24148 24250 +102
Branches 4102 4118 +16
==========================================
- Hits 5026 4590 -436
- Misses 18792 19384 +592
+ Partials 330 276 -54
Continue to review full report at Codecov.
|
Create a new application for the creation of isosurfaces around data values.