Add compas_3dec solver integration - #23
Conversation
| >>> problem = Problem(model) | ||
| >>> problem.add_gravity() | ||
| >>> problem.solver(Solver.CRA()) # doctest: +SKIP | ||
| >>> _ = problem.add_boundary_condition("gravity").add_gravity() |
There was a problem hiding this comment.
if you add # doctest: +SKIP the awkward _ assignment is not necessary
There was a problem hiding this comment.
I removed the _ assignment.
|
@tomvanmele I added one follow-up commit after the initial approval:
All checks pass locally: Ruff passed and pytest reported 7 passed, 1 skipped. Could you please briefly re-review the latest commit before merging? |
tomvanmele
left a comment
tomvanmele
left a comment
There was a problem hiding this comment.
i recognize typical LLM interventions...
|
|
||
| # These values live in Problem.solver and are picked up by | ||
| # ThreeDECAnalysisBuilder when it creates the portable snapshot. | ||
| _ = (ratio, ratio_keyword, time, gravity_steps, stages) |
There was a problem hiding this comment.
I removed the dummy assignment and now pass the ratio, ratio keyword, time, gravity steps and optional stages explicitly to the prepared 3DEC analysis.
| @@ -150,4 +150,3 @@ def compute_obb(self, inflate: float = 1.0) -> Box: | |||
|
|
|||
| def compute_point(self) -> Point: | |||
| return Point(*self.modelgeometry.centroid()) | |||
There was a problem hiding this comment.
i don't think it is necessary convert. return of centroid() is already a point object
There was a problem hiding this comment.
Indeed, changed. Now, it is not constructing another Point.
| -------- | ||
| >>> bc = BoundaryConditionGroup(name="Live") | ||
| >>> bc.add_point_load(block_index=10, force=[0, 0, -5000]) | ||
| >>> _ = bc.add_point_load(block_index=10, force=[0, 0, -5000]) |
There was a problem hiding this comment.
i don't like this kind of thing. i assume it is to avoid a doctest error. if there is a meaningful return object, just assign it to a meaningful variable...
There was a problem hiding this comment.
Fixed. The returned point load is now assigned to a point_load variable and used in the example.
| >>> problem = Problem(model) | ||
| >>> problem.add_gravity() | ||
| >>> problem.solver(Solver.CRA()) # doctest: +SKIP | ||
| >>> _ = problem.add_boundary_condition("gravity").add_gravity() |
| from compas_dem.analysis.lmgc90 import lmgc90_solve | ||
|
|
||
| return lmgc90_solve(self, model, **params) | ||
| elif solver.name in ("3DEC", "threeDEC"): |
There was a problem hiding this comment.
when using returns in your ifs, elifs are not necessary...
| return self | ||
|
|
||
| @staticmethod | ||
| def _validate_threedec_stages(stages): |
There was a problem hiding this comment.
static methods that are not part of a public API (which would be a valid reason to add it to the class even though it doesn't interact with it), should not be methods, but rather private module scope functions
There was a problem hiding this comment.
I moved the stage-validation function to module level.
|
@tomvanmele please review. Thanks |
2 participants
Summary
Integrate
compas_3decas a solver backend forcompas_dem.Changes
Solver.ThreeDEC(...)configuration.Problem.solve()to the ThreeDEC adapter.compas_dem.ProblemwithThreeDECAnalysisBuilder.from_dem_problem(problem).build().ThreeDECSolver.raw_results.to_compas_dem_results(analysis).DEMViewervisualisation.Verification
DEMViewer.Notes
compas_3decremains an optional dependency. The real solver smoke test isskipped unless
COMPAS_3DEC_EXECUTABLEis configured.