First merge request #8
Conversation
…ry through the GeoTessModel class
|
This is awesome, Rob. I'm looking it over now. Pinging @sandyballard505 for visibility. |
There was a problem hiding this comment.
This is great stuff, @rwporritt ! Thank you for such a large addition to the interface. It's got enough in there probably for more than one PR, so I apologize if my review feels like it's all over the place.
- You've done a ton of great work exposing more of the GeoTessCPP interface. So much, in fact, that I think there's a lot for me to learn about Cython in here:-)
- There are a number of places where you're adding methods that extend the C++ class interfaces. I really like these, but it warrants a conversation (real-time, probably) and some planning about where it makes the most sense to add these. I have the h5py API in mind, where they've mirrored the C-level HDF5 API (making some conventions about input/output types) in one layer of the package, and they've also extended that low-level API into a higher-level interface that does more things and does them in a way that looks more familiar to a Python programmer.
- 1 and 2 are already kind of a lot, but I'd love to reach out to @sandyballard505 (or someone familiar with GeoTessCPP) to help mirror the tests from that project into this one. I don't see them in the original code base, and I think it'd really help make some of the discussions in 1 & 2 more concrete.
Thank you again, and I look forward to hearing your thoughts.
| @@ -0,0 +1,456 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Since GeoTessBuilder.py doesn't appear to use the C++ library, but uses the Java jar file, I suggest either moving it to the GeoTessJava repository or starting its own repo.
| * | ||
| * @return int | ||
| */ | ||
| int getTypeInt() { |
| @@ -0,0 +1,11 @@ | |||
| gridConstructionMode = Uniform | |||
There was a problem hiding this comment.
If this file is in support of GeoTessBuilder.py, I suggest moving this one too.
| Extracts from geotess object to a set of 3 location vectors and an attribute matrix | ||
| returns longitude vector, latitude vector, radius vector, and data matrix | ||
| """ | ||
| import numpy as np |
There was a problem hiding this comment.
Please move to top of the file, next to other imports.
| pos.set(lat, lon, depth) | ||
| return str(pos.toString()) | ||
|
|
||
| def positionToStringLayer(self, layerid, lat, lon, depth, horizontalType="LINEAR", radialType="LINEAR"): |
There was a problem hiding this comment.
A number of the following methods also look like they aren't currently in the existing CPP API, and are good candidates for discussing whether they make sense to include here or in a separate Pythonic API layer.
| relies on numpy as np | ||
|
|
||
| """ | ||
| import numpy as np |
There was a problem hiding this comment.
Move import to top of file, next to other imports.
|
|
||
| # Should get this from GeoTessModelUtils | ||
| # Needs an update based on updated getGeographicLocationAttribute() method | ||
| def makeDepthMap(self, float depth, int attribute, int layer, float dLon = 8.0, |
There was a problem hiding this comment.
Another candidate for a different location. I'm thinking geotess/model.py at the moment.
|
|
||
| return depths, outData | ||
|
|
||
| def convertToNPArray(self): |
There was a problem hiding this comment.
This feels like a utility function that could reside in geotess/utils.py.
|
I have looked through this and I am not sure I totally get what is going on. I would be happy to participate in a call to discuss the path forward.
There is an extensive set of tests for the c++ and c interfaces. I’ve attached the c++ tests.
Sandy
|
|
I’ve look through it a couple times and am also a bit overwhelmed. Probably
my fault for pushing such a big update.
I’m off tomorrow, but would be happy to chat next week.
|
|
You guys are writing python code to wrap essentially every public c++ method? That would be a lot.
|
|
Sounds good, Rob. We can touch base next week. BTW, it looks like yours and Sandy's GitHub settings are such that the entire body of the emails you're sending are showing up in the conversation of the PR. Further, Sandy, your attachment is stripped so I don't have it:-( I'm going to remove them from your responses on GitHub because it's making the PR page really hard to follow, but I recommend removing the previous email body in your responses or just responding on the PR page. |
Here's the first push to get most of the functionality together. I have a couple notebooks that we can add too.