dFEM: explicit dynamics gpu example #1465
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1465 +/- ##
===========================================
+ Coverage 92.03% 92.06% +0.03%
===========================================
Files 199 200 +1
Lines 25212 25356 +144
===========================================
+ Hits 23203 23344 +141
- Misses 2009 2012 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| set(SERAC_ENABLE_CODEVELOP ON CACHE BOOL "") | ||
|
|
||
| set(SERAC_DISABLE_TRIBOL ON CACHE BOOL "") |
There was a problem hiding this comment.
imo, it would be better to unset TRIBOL_DIR here, or at least rename this var to SERAC_ENABLE_TRIBOL to avoid double-negative
There was a problem hiding this comment.
I'll have a separate tribol-related PR up soon that will eliminate the need for this variable.
ebchin
added
ready for review
| # | ||
| # SPDX-License-Identifier: (BSD-3-Clause) | ||
|
|
||
| if(SMITH_USE_DFEM) |
There was a problem hiding this comment.
it seems like SMITH_USE_DFEM isn't associated with any package, but rather its to enable or disable certain tests/ examples. maybe SMITH_ENABLE_DFEM is more appropriate?
i still don't fully understand the difference between dfem and enzyme, so maybe this question doesn't make any sense, but could we use SMITH_USE_ENZYME here?
also last question, could this option be removed once #1477 merges?
There was a problem hiding this comment.
Yes, this bugged me too. I like SMITH_ENABLE_DFEM more, but it looks so out of place here: https://github.com/LLNL/smith/blob/develop/src/smith/smith_config.hpp.in#L32-L55. Also, we can make it go away once #1477 is merged. Maybe we just keep it as SMITH_USE_DFEM for now and get rid the variable when #1477 merges?
There was a problem hiding this comment.
A few more thoughts:
Re: SMITH_USE_ENZYME, you could use dFEM without Enzyme, so it doesn't quite make sense here.
Also, IMO, using the SMITH_ENABLE_DFEM name would make sense at the CMake level (i.e. do conditional header includes instead of doing a #cmakedefine SMITH_USE_DFEM), but it breaks the header check in CI.
There was a problem hiding this comment.
its fine to leave it as is then, and we can remove it later. may be worth creating an issue to track it
chapman39
left a comment
chapman39
left a comment
There was a problem hiding this comment.
just had a couple questions about SMITH_USE_DFEM option (see above) besides that lgtm
Co-authored-by: Alex Tyler Chapman <100869159+chapman39@users.noreply.github.com>
This is an attempt to bring the dFEM functionality online in smaller, more easily reviewable chunks.
This first PR enables required dFEM functionality to solve a forward explicit dynamics problem. Parameters, jvp, and vjp functionality will likely be the next chunk.
Note: waiting for #1469 to merge before merging this one in