StudioInfinity/dyna3
2
0
Fork 2
Code Issues 57 Pull requests 1 Projects 3 Releases 4 Wiki Activity Actions

Simplify the realization triggering system #105

Merged
glen merged 6 commits from Vectornaut/dyna3:reactive-realization-cleanup into main 2025-07-31 22:21:34 +00:00
Conversation 2 Commits 6 Files changed 4 +48 -104
Vectornaut commented 2025-07-24 22:19:57 +00:00
Member
Copy link

Overview

The incoming branch simplifies the system that reactively triggers realizations, at the cost of removing the preconditioning step described in issue #101 and doing unnecessary realizations after certain kinds of updates.

The new system should trigger a realization after any update that could affect the assembly's deformation space. For simplicity, any update to the regulator list triggers an update, even if it doesn't affect the set of constraints. In particular, adding a regulator triggers an unnecessary realization.

Future directions

Preconditioning

Issue #101 will remind us to replace the preconditioning functionality that this pull request removes.

Unnecessary realizations

The main cost of an unnecessary realization is probably recomputing the tangent space of the solution variety. If we keep the realization triggering system from this pull request, we should keep an eye on the cost of this computation and decide whether it's possible to speed it up, background it, or do it only as needed. We currently use or expect to use tangent space information for:

  • Deforming the assembly.
  • Reporting how many degrees of freedom the assembly has.
### Overview The incoming branch simplifies the system that reactively triggers realizations, at the cost of removing the preconditioning step described in issue #101 and doing unnecessary realizations after certain kinds of updates. The new system should trigger a realization after any update that could affect the assembly's deformation space. For simplicity, any update to the regulator list triggers an update, even if it doesn't affect the set of constraints. In particular, adding a regulator triggers an unnecessary realization. ### Future directions #### Preconditioning Issue #101 will remind us to replace the preconditioning functionality that this pull request removes. #### Unnecessary realizations The main cost of an unnecessary realization is probably recomputing the tangent space of the solution variety. If we keep the realization triggering system from this pull request, we should keep an eye on the cost of this computation and decide whether it's possible to speed it up, background it, or do it only as needed. We currently use or expect to use tangent space information for: - Deforming the assembly. - Reporting how many degrees of freedom the assembly has.
Vectornaut added 1 commit 2025-07-24 22:19:58 +00:00
Trigger realization more directly
Some checks failed
/ test (pull_request) Failing after 1m36s
Details
3eddef784b 
Simplify the system that reactively triggers realizations, at the cost
of removing the preconditioning step described in issue #101 and doing
unnecessary realizations after certain kinds of updates.

The new system should trigger a realization after any update that could
affect the dimension of the assembly's deformation space. For
simplicity, any update to the regulator list triggers an update, even if
it doesn't affect the set of constraints. In particular, adding a
regulator triggers an unnecessary realization.
Vectornaut force-pushed reactive-realization-cleanup from 3eddef784b to c73008d702 2025-07-24 22:23:46 +00:00 Compare
Vectornaut changed title from Simplify the realization triggering system to WIP: Simplify the realization triggering system 2025-07-24 22:35:29 +00:00
Vectornaut added 2 commits 2025-07-24 23:33:17 +00:00 
Add a reminder to remove the workaround once the bug is fixed.
Prevent unused imports for engine debug output
All checks were successful
/ test (pull_request) Successful in 3m38s
Details
2bae8d3df9 
In the process, switch from the `web-sys` crate's `console::log_1`
function to Sycamore's more flexible `console_log` macro. The latter
works both inside and outside the browser, so we can use it without
checking whether we're compiling to WebAssembly.
Vectornaut changed title from WIP: Simplify the realization triggering system to Simplify the realization triggering system 2025-07-25 16:33:17 +00:00
glen reviewed 2025-07-26 15:43:40 +00:00
app-proto/src/engine.rs Outdated
@ -206,2 +173,2 @@
format!("Eigenvalues used to find kernel:{}", eig.eigenvalues)
));
#[cfg(not(feature = "dev"))]
console_log!("Eigenvalues used to find kernel:{}", eig.eigenvalues);
glen commented 2025-07-26 15:42:11 +00:00
Owner
Copy link

What does this #-directive do? Not compile the next line if this is not a "dev" compile? Why do we not want to log the eigenvalues on a dev build? More likely I am misunderstanding the meaning here...

What does this #-directive do? Not compile the next line if this is not a "dev" compile? Why do we not want to log the eigenvalues on a dev build? More likely I am misunderstanding the meaning here...
Vectornaut commented 2025-07-28 17:24:19 +00:00
Author
Member
Copy link

What does this #-directive do? Not compile the next line if this is not a "dev" compile?

Correct, if you replace "not compile" with "only compile" (I think this was a typo, based on your next sentence) and "line" with the various language constructs that attributes can be applied to.

Why do we not want to log the eigenvalues on a dev build?

Short answer

The eigenvalue logging is part of the function ConfigSubspace::symmetric_kernel, which currently runs at the end of every realize_gram call. This messes up the formatting of the Cargo examples, which are built with the dev feature.

If we want to add eigenvalue logging to the example output, we'd have to reorganize the Cargo examples a little bit, which seems a bit outside the scope of this pull request.

Now that we have a diagnostics panel, it seems more useful to plot the eigenvalues there than to log them to the console, so this logging code's days should be numbered. As we discussed during our last meeting, I'm planning to experiment with a new implementation of ConfigSubspace::symmetric_kernel this week. If we adopt a new implementation, it will be a good opportunity to replace the console logging code with a diagnostics panel plot.

More context

On the main branch, the eigenvalue logging uses the console::log_1 function from the web-sys crate, which can only run in a browser. It's therefore confined to WebAssembly builds using the following attribute:

#[cfg(all(target_family = "wasm", target_os = "unknown"))]

Since the examples typically aren't built for a WebAssembly target, they don't include the eigenvalue logging code.

This pull request seemed like a good opportunity to switch to Sycamore's more flexible console_log macro, which works both inside and outside the browser. I've been gradually replacing console::log_/* number */ calls with console_log calls throughout the codebase. The eigenvalues now can be logged in the Cargo examples, but I decided to keep the eigenvalue logging out of the examples to preserve existing behavior.

> What does this #-directive do? Not compile the next line if this is not a "dev" compile? Correct, if you replace "not compile" with "only compile" (I think this was a typo, based on your next sentence) and "line" with the [various language constructs](https://doc.rust-lang.org/reference/attributes.html#r-attributes.allowed-position) that [attributes](https://doc.rust-lang.org/reference/attributes.html) can be applied to. > Why do we not want to log the eigenvalues on a dev build? #### Short answer The eigenvalue logging is part of the function `ConfigSubspace::symmetric_kernel`, which currently runs at the end of every `realize_gram` call. This messes up the formatting of the Cargo examples, which are built with the `dev` feature. If we want to add eigenvalue logging to the example output, we'd have to reorganize the Cargo examples a little bit, which seems a bit outside the scope of this pull request. Now that we have a diagnostics panel, it seems more useful to plot the eigenvalues there than to log them to the console, so this logging code's days should be numbered. As we discussed during our last meeting, I'm planning to experiment with a new implementation of `ConfigSubspace::symmetric_kernel` this week. If we adopt a new implementation, it will be a good opportunity to replace the console logging code with a diagnostics panel plot. #### More context On the main branch, the eigenvalue logging uses the `console::log_1` function from the `web-sys` crate, which can only run in a browser. It's therefore confined to WebAssembly builds using the following attribute: ```rust #[cfg(all(target_family = "wasm", target_os = "unknown"))] ``` Since the examples typically aren't built for a WebAssembly target, they don't include the eigenvalue logging code. This pull request seemed like a good opportunity to switch to Sycamore's more flexible `console_log` macro, which works both inside and outside the browser. I've been gradually replacing `console::log_/* number */` calls with `console_log` calls throughout the codebase. The eigenvalues now *can* be logged in the Cargo examples, but I decided to keep the eigenvalue logging out of the examples to preserve existing behavior.
glen commented 2025-07-28 19:18:02 +00:00
Owner
Copy link

It feels to me as though we are using compile time directives to make a runtime choice about logging, because by coincidence we don't want logging in the examples which happen to be our only non-dev builds at the moment. That mechanism doesn't seem fitted to the purpose. Could we select logging or not in another way? Or just cut this particular logging altogether if we don't really need it anyway?

It feels to me as though we are using compile time directives to make a runtime choice about logging, because by coincidence we don't want logging in the examples which happen to be our only non-dev builds at the moment. That mechanism doesn't seem fitted to the purpose. Could we select logging or not in another way? Or just cut this particular logging altogether if we don't really need it anyway?
Vectornaut commented 2025-07-28 21:20:13 +00:00
Author
Member
Copy link

I'm fine with dropping the eigenvalue logging now and replacing it with a diagnostics panel plot later—either in a stand-alone pull request or as part of a pull request that revises ConfigSubspace::symmetric_kernel.

I'm fine with dropping the eigenvalue logging now and replacing it with a diagnostics panel plot later—either in a stand-alone pull request or as part of a pull request that revises `ConfigSubspace::symmetric_kernel`.
Vectornaut commented 2025-07-29 08:07:06 +00:00
Author
Member
Copy link

I've dropped the eigenvalue logging code (in commit eafb133) and filed an issue (#106) to remind us to replace it.

I've dropped the eigenvalue logging code (in commit eafb133) and filed an issue (#106) to remind us to replace it.
glen marked this conversation as resolved
app-proto/src/engine.rs Outdated
@ -428,0 +403,4 @@
);
return Realization { result, history }
}
glen commented 2025-07-26 15:43:33 +00:00
Owner
Copy link

(a) Why do we now need an empty-assembly special case, when we didn't before? Or is it just for efficiency?

(b) Could this be expanded relatively easily to the more common no-set-regulators case?

(a) Why do we now need an empty-assembly special case, when we didn't before? Or is it just for efficiency? (b) Could this be expanded relatively easily to the more common no-set-regulators case?
Vectornaut commented 2025-07-28 17:57:18 +00:00
Author
Member
Copy link

(a) Why do we now need an empty-assembly special case, when we didn't before? Or is it just for efficiency?

The realization triggering system on the main branch never tries to realize an empty assembly, so we've never had to worry about the empty-assembly case.

As I mentioned in the pull request description, the realization triggering system on the incoming branch allows some unnecessary realizations in exchange for simplicity. One of those unnecessary realizations happens when we construct an empty assembly using the constructor Assembly::new.

As it turns out, the realization function on the main branch panics when you try to realize an empty assembly. This is because an empty assembly has an empty (zero by zero) Hessian, and the DMatrix::from_columns function, which we use to build the Hessian, can't handle an empty list of columns.

(b) Could this be expanded relatively easily to the more common no-set-regulators case?

Sure. We could give PartialMatrix an is_empty method and then expand the early return condition to:

assembly_dim == 0 || (gram.is_empty() && frozen.is_empty())
> (a) Why do we now need an empty-assembly special case, when we didn't before? Or is it just for efficiency? The realization triggering system on the main branch never tries to realize an empty assembly, so we've never had to worry about the empty-assembly case. As I mentioned in the pull request description, the realization triggering system on the incoming branch allows some unnecessary realizations in exchange for simplicity. One of those unnecessary realizations happens when we construct an empty assembly using the constructor `Assembly::new`. As it turns out, the realization function on the main branch panics when you try to realize an empty assembly. This is because an empty assembly has an empty (zero by zero) Hessian, and the `DMatrix::from_columns` function, which we use to build the Hessian, can't handle an empty list of columns. > (b) Could this be expanded relatively easily to the more common no-set-regulators case? Sure. We could give `PartialMatrix` an `is_empty` method and then expand the early return condition to: ```rust assembly_dim == 0 || (gram.is_empty() && frozen.is_empty()) ```
glen commented 2025-07-28 19:21:03 +00:00
Owner
Copy link

Any reason not to go ahead and do that in this PR? Seems like more bang for not many more bucks -- as long as we are checking for one kind of trivial case, why not exclude many more trivial cases? Seems to me like there's no downside...

Any reason not to go ahead and do that in this PR? Seems like more bang for not many more bucks -- as long as we are checking for one kind of trivial case, why not exclude many more trivial cases? Seems to me like there's no downside...
Vectornaut commented 2025-07-28 21:55:26 +00:00
Author
Member
Copy link

Any reason not to go ahead and do that in this PR?

On second thought, we'd actually have to use a separate conditional for the unconstrained case, because the tangent space would be different. When there are no elements, we return the zero subspace as the tangent space; when there are elements but no constraints, we want to return the whole configuration vector space as the tangent space.

I'd recommend leaving realize_gram mostly as it is. When there are no constraints, realize_gram still exits before doing a Cholesky decomposition, which is the most costly part of the optimization step: the loop breaks halfway through the first iteration, in the block with the comment "stop if the loss is tolerably low." After the break, we use ConfigSubspace::symmetric_kernel to find the tangent space. This costs more computationally than hard-coding the tangent space for the unconstrained case, but I think it makes maintenance easier.

Note that to get the spectrum for the diagnostics panel, realize_gram finds the eigenvalues of the Hessian on every step. I'm not factoring this into the cost considerations above, because I'd only want to do this in development builds.

> Any reason not to go ahead and do that in this PR? On second thought, we'd actually have to use a separate conditional for the unconstrained case, because the tangent space would be different. When there are no elements, we return the zero subspace as the tangent space; when there are elements but no constraints, we want to return the whole configuration vector space as the tangent space. I'd recommend leaving `realize_gram` mostly as it is. When there are no constraints, `realize_gram` still exits before doing a Cholesky decomposition, which is the most costly part of the optimization step: the loop breaks halfway through the first iteration, in the block with the comment "stop if the loss is tolerably low." After the break, we use `ConfigSubspace::symmetric_kernel` to find the tangent space. This costs more computationally than hard-coding the tangent space for the unconstrained case, but I think it makes maintenance easier. Note that to get the spectrum for the diagnostics panel, `realize_gram` finds the eigenvalues of the Hessian on every step. I'm not factoring this into the cost considerations above, because I'd only want to do this in development builds.
glen commented 2025-07-28 23:13:46 +00:00
Owner
Copy link

@Vectornaut wrote in #105 (comment):

On second thought, we'd actually have to use a separate conditional for the unconstrained case, because the tangent space would be different. When there are no elements, we return the zero subspace as the tangent space; when there are elements but no constraints, we want to return the whole configuration vector space as the tangent space.

How do those differ? When there are no elements, the whole configuration vector space is the zero space...

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/105#issuecomment-3114: > On second thought, we'd actually have to use a separate conditional for the unconstrained case, because the tangent space would be different. When there are no elements, we return the zero subspace as the tangent space; when there are elements but no constraints, we want to return the whole configuration vector space as the tangent space. How do those differ? When there are no elements, the whole configuration vector space is the zero space...
Vectornaut commented 2025-07-29 17:19:59 +00:00
Author
Member
Copy link

How do those differ? When there are no elements, the whole configuration vector space is the zero space...

Good point: in principle, a ConfigSubspace variant representing a full subspace could be used for both empty assemblies and unconstrained assemblies. I've prototyped a full subspace representation and saved the code locally in case we want it later. In the process, however, I realized that we'll never actually encounter an unconstrained assembly, because every element comes with a normalization constraint. I'm now strongly against adding special handling for unconstrained assemblies, because during normal operation, this code will never run.

> How do those differ? When there are no elements, the whole configuration vector space is the zero space... Good point: in principle, a `ConfigSubspace` variant representing a full subspace could be used for both empty assemblies and unconstrained assemblies. I've prototyped a full subspace representation and saved the code locally in case we want it later. In the process, however, I realized that we'll never actually encounter an unconstrained assembly, because every element comes with a normalization constraint. I'm now strongly against adding special handling for unconstrained assemblies, because during normal operation, this code will never run.
glen commented 2025-07-29 19:36:57 +00:00
Owner
Copy link

OK, yes, I had forgotten that our coordinates are themselves underdetermined and that we use essentially the same mechanism to keep the coordinates for a given item in the form we want as to place an actual Euclidean-geometric constraint. So I agree, there's no point in any code for unconstrained assemblies other than the totally empty assembly. So yes go ahead and leave the special case for empty assemblies; please just add a comment as to why it must be handled specially. Thanks!

OK, yes, I had forgotten that our coordinates are themselves underdetermined and that we use essentially the same mechanism to keep the coordinates for a given item in the form we want as to place an actual Euclidean-geometric constraint. So I agree, there's no point in any code for unconstrained assemblies other than the totally empty assembly. So yes go ahead and leave the special case for empty assemblies; please just add a comment as to why it must be handled specially. Thanks!
Vectornaut commented 2025-07-29 20:52:35 +00:00
Author
Member
Copy link

So yes go ahead and leave the special case for empty assemblies; please just add a comment as to why it must be handled specially. Thanks!

Done in commit 779c026!

> So yes go ahead and leave the special case for empty assemblies; please just add a comment as to why it must be handled specially. Thanks! Done in commit 779c026!
glen marked this conversation as resolved
glen commented 2025-07-26 15:44:14 +00:00
Owner
Copy link

Mostly looks just fine, a couple of questions and then we should be good to go.

Mostly looks just fine, a couple of questions and then we should be good to go.
Vectornaut added 1 commit 2025-07-28 17:46:12 +00:00
Correct the indentation of an empty line
All checks were successful
/ test (pull_request) Successful in 3m36s
Details
ca57fbce86
Vectornaut added 1 commit 2025-07-29 07:45:52 +00:00
Drop eigenvalue logging from symmetric_kernel
All checks were successful
/ test (pull_request) Successful in 3m30s
Details
eafb133f8d
Vectornaut added 1 commit 2025-07-29 20:48:50 +00:00
Explain why the empty-assembly case is special
All checks were successful
/ test (pull_request) Successful in 3m35s
Details
779c0260bb
glen merged commit 2eba80fb69 into main 2025-07-31 22:21:34 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
design

Issues that entail architecture/design decisions

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
maintenance

refactors andother behavior-orthogonal tasks

  
prospective

this becomes an issue once a particular feature is implemented or pull request is merged

  
question

More information is needed

  
regression

A bug that's resurfaced or a feature that's accidentally been removed

  
stub

We need to fill in the details of this issue

  
todo

Planned step in implementation

  
ui

issue concerning the user interface

  
wontfix

This won't be fixed

No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: StudioInfinity/dyna3#105
No description provided.
Delete branch "Vectornaut/dyna3:reactive-realization-cleanup"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?