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
Add spherical shell geometry support to DEM components #320
Conversation
b7e4b9d
to
f369817
Compare
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.
Hi @schunkes, this is a first very quick review. Key points:
- Please make the scope of this PR clear and as limited as possible, i.e. stick to extending
DEMExperiment
with spherical shell geometry support. - Please review the whole DEM processing code and vectorize it, it will be clearer, more compact and much faster.
f369817
to
d6b8bc8
Compare
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.
Hi @schunkes, thanks for proceeding with vectorization. It looks like a round of cleanup is required prior to proceeding with fine tuning, notably paying attention to:
- unit handling;
- docstring and annotation agreement.
Looking again at this, I think we are in a complicated situation where file buffering fundamentally breaks unit support (we already worked around this but it's not bulletproof).
Also, passing through a file write is something we've been trying to avoid and could be delegated to the user without losing too much convenience. The pattern would then be something like this (still breaking unit support):
# Tesselate DEM
dem_mesh: mi.Mesh = make_dem_from(some_data)
# Either use it directly,
dem_surface = DEMSurface(shape=dem_mesh, ...)
# Or through a file
dem_mesh.write_ply("dem.ply")
dem_surface = DEMSurface(shape="dem.ply")
Let's review your code live after you're done cleaning it up a bit.
@leroyvn would you mind, taking another look at the code now? I updated it according to what we discussed. Additionally, I gave the BufferMesh a I did not update the tests yet, so do not expect them to work or pass. |
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.
Hi @schunkes, we're getting there on the design side. There are a lot of details to fix. I've been specific, but I probably missed some. Please:
- Check defaults and type annotations. There are many missing or inconsistent entries. You also sometimes use mutables as defaults, which is very dangerous.
- Check unit requirement consistency in all your functions. To keep it simple and safe, either all args should have units, or none (and then you might have to document arg units).
- Please compile and check the documentation.
242d7a1
to
7eb79db
Compare
a1fd5ec
to
90ba378
Compare
All things we discussed are implemented. I think this can be reviewed again. |
c5021ec
to
77a3d40
Compare
I applied a round of fixes. Main changes are:
|
Looks good :) |
Description
This PR updates DEM-related components to allow using a spherical shell geometry with the
DEMExperiment
class.Checklist
main
branch