feat: Engine diagnostics #92
No reviewers
Labels
No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: StudioInfinity/dyna3#92
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Vectornaut/dyna3:diagnostics"
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?
Built on top of pull request #91.
Primary changes
To facilitate engine development in the application prototype, add a
Diagnosticscomponent that shows the following diagnostics from the last realization:The loss and spectrum plots are shown on switchable panels, which are designed with extensibility in mind. When we're tinkering with the engine, we should be able to add or swap out diagnostics panels as needed.
Secondary changes
Encapsulate realization results and diagnostics in the new
RealizationResultstructure. In the process, spruce up our realization diagnostics logging and factor out some of the repetitive code in the examples, because we're already changing those parts of the code to adapt them to the new encapsulation.This change, carried out in commit
d4302d2, affects the output format of the Cargo examples. I've checked by hand that the output is rearranged but not meaningfully changed.feat: Engine diagnosticsto WIP: feat: Engine diagnosticsI decided to include the scale change we discussed on Monday in this pull request. It's implemented in commit
2688b76(formerlyfa39a9a). As proposed, the Spectrum chart now splits the spectrum of the Hessian into its positive and negative parts and plots them on the same logarithmic axis, throwing away eigenvalues whose sizes are below a certain threshold (currently10^{-6}). The data zoom controls are still linearly scaled, as discussed in ECharts issue #20927.WIP: feat: Engine diagnosticsto feat: Engine diagnostics@Vectornaut wrote in #92 (comment):
This is to protect us from trying to display exactly-zero eigenvalues, that have no log? Should you in some way display the cutoff and the number of elided eigenvalues? (We are interested in the dimensionality of the solution variety, which is connected with that number, at least at a true solution, if I am not mistaken.)
Yes. Trying to display eigenvalue which are too close to zero seems to throw off the scaling procedure that ECharts uses, leading to a lot of eigenvalues being cut out of the chart window.
I was considering adding a component that would count the too-close-to-zero eigenvalues, and I could try implementing it if you'd like.
True, but the rule we use to classify eigenvalues as zero when computing the tangent space is different from the one I'm using here to avoid chart scaling problems, and I could imagine them becoming even more different in the future. For example, if we switch to a rule that involves looking for a gap in the eigenvalue distribution, that would probably allow eigenvalues to be declared nonzero while still being small enough to throw off the chart scaling.
@Vectornaut wrote in #92 (comment):
I guess I had imagined something like plotting a numeral at or just below the cutoff spot in the eigenvalue display, rather than a separate component. If that's relatively easy with the current code, then sure, go ahead and put it in this PR. If it would be involved and you'd rather defer, let me know and I will file an issue after this is merged.
As for the difference between a "too-small-to-plot" eigenvalue and a "inferred-to-be-zero" eigenvalue (for the sake of the tangent space), it would be nice at some point to coalesce them. I don't see any reason that we couldn't other than shortcomings to the plotting software, given that we are on a log scale anyway. (E.g., if we're using a wonky log-scale feature of the plotting software at the moment, maybe we could just take the logs ourselves and give the data to the plotter as log values to be plotted linearly?) Anyhow, I am not fussed by this discrepancy at the moment -- happy to just file a low-priority issue about it once this is merged.
This turns out to be a nice workaround for both the scaling problem and the problem with data zoom controls being linear. I've implemented it in commit
e877985(formerlyc2c1e4a). Now all eigenvalues are displayed except ones that are exactly zero (which I do think I've seen occasionally).Is it worth trying to show the presence of exact zero eigenvalues? I could do it by adding an extra axis to the chart, but I'm not convinced it'd be worth the area it would take up.
@Vectornaut wrote in #92 (comment):
Well, the easiest way would be to show a symbol (or numeral giving multiplicity) in a different color or something like that, at some reasonable offset below the smallest nonzero eigenvalue. In other words, if the logs of the nonzero eigenvalues ranged from 3 to -7, you could put them at something like -10 but displayed in a way that distinguishes them. Or you could put a small text caption somewhere like "And 2 machine-zero values". I will leave it up to you whether to bother with something like that in this PR. I agree it's not worth an extra axis.
I guess we don't need an on-screen or README/other doc-file reminder that the positive eigenvalues are the blue dots and the negative ones the green squares? (I guessed from the distribution of the problem I tried, but then looked it up in the code to be sure...)
Done in commit
68d6cc1(formerly4765acf).I could start a wiki page that briefly explains the diagnostics. (Right now, the series colors are chosen automatically, so it might be best to refer only to the series shapes.)
@Vectornaut wrote in #92 (comment):
Do you by any chance have an assembly I can set up that exercises this?
Yes—strictly-zero eigenvalues actually seem to turn up often. As an example, go to the General test assembly, set the inversive distance between Castor and Pollux to 0.5, and look at the spectrum history. You'll see strictly-zero eigenvalues at every step of the realization process. I've confirmed that this is accurate by printing the eigenvalues at each step.
4765acf6b9to68d6cc1645The incoming branch is now rebased onto the newly merged #91, so it's ready to merge, pending review!
The file name
app-proto/examples/common/mod.rs(well, the "mod.rs" part specifically) is inscrutable to me. Please could you rename it to something more descriptive? It looks something like "common/diagnostic.rs" or "common/log.rs" or "common/display.rs" or something, but perhaps I've misunderstood -- I really couldn't think what word "mod" was supposed to be short for. Thanks! (I just started reading the code, so there may be other comments.)The file
app-proto/run-exampleshas gotten more code-y in this PR, begging the question as to what command interpreter is intended to run it. Please rename it to make that clear from the file name, e.g. "run-examples.sh" or "run-examples.bash" or something. Thanks!@glen wrote in #92 (comment):
Putting a file called
mod.rsin a source directory is one way to create a module. However, Rust has apparently introduced a new module filename system which is more readable, so I'll switch to that!Otherwise not much, as this is mostly new display code with a bit of bookkeeping. Just some naming questions.
@ -28,0 +27,4 @@ConstraintProblem,DescentHistory,Realization,RealizationResultDoes Rust allow a final comma here? If so, please include one, to avoid in the future the semantically spurious first line of diff ("ConstraintProblem "-> "ConstraintProblem,")
Yes, the MIT comma is allowed in Rust, and recommended in the Rust style guide. I started writing the app prototype without trailing commas, with the intention of evaluating how annoying that style is, and by now I'm convinced that trailing commas would be nicer. Could we keep this pull request consistent with the current style and then do a small pull request that adds trailing commas throughout the code?
OK, fine, file an issue to do it (label "maintenance").
@ -559,1 +566,3 @@elements_by_id: create_signal(BTreeMap::default())elements_by_id: create_signal(BTreeMap::default()),realization_status: create_signal(Ok(())),descent_history: create_signal(DescentHistory::new())Conceptually, does the descent history belong as a member of an Assembly?
My perspective here is that realization happens within an assembly, so its results—including the descent history—belong to the assembly.
The upcoming history display pull feature will force us to rethink that, because we decided to implement that feature by turning the realization history into a whole stack of assemblies—one for each descent step.
I propose keeping the current organization for this pull request, because it's consistent with the way I've been thinking about realization, and then reorganizing while we write the history display feature.
Yes, this is fine -- I think something closer to my perspective is that an assembly is a geometric arrangement together with a list of properties it is intended to have (i.e., constraints). I think that will be consistent with the reorganization you propose.
@ -396,0 +398,4 @@pub tangent: ConfigSubspace}pub struct RealizationResult {The difference between a "Realization" and a "RealizationResult" is not at all clear to me. I guess that this is some Rust-ism, in that a Result is some customary wrapper around a value, in this case a Realization -- is that right? Do you think the two names are completely idiomatic in a Rust world? Or do you think there might be some plain-english names that would better convey what's going on? (Intuitively to me, it would seem a Realization would be the natural outcome of trying to realize an Assembly...)
Trying to realize an assembly doesn't always produce a realization (because it can fail), and it also produces additional data (the descent history and other diagnostic information). Are you hoping for a better way to organize this data, or just a more descriptive name for the data structure? If the latter, what do you think of
RealizationOutcomeorRealizationOutput?No, I don't think the current naming is idiomatic. In fact, I find it a little misleading, because I'd expect something called
RealizationResultto just be or contain aResult<Realization, _>, with no additional data.OK, so currently a Realization is just a set of coordinates for everything, is that right? The other gadget is all the stuff that can be produced from a call to a "realize" function that tries to create a Realization that satisfies all the constraints, but (a) might fail, and (b) produces additional info, correct?
So yes, I guess all I am looking for are better names for these two things. Here are some options:
(a) What is currently called a Realization should perhaps just be called a Configuration -- if it is just coordinates, there's no guarantee that this data structure "realizes" any particular set of constraints. That would free up "Realization" to be, quite naturally, the result of calling
realize()-- and such a thing would naturally include a Configuration if it managed to find one. (But again, I don't quite understand why it wouldn't always include a Configuration (the best it could do) and a flag to tell you whether this configuration actually realized all the constraints.)(b) If you feel strongly that the collection of coordinates thing should absolutely be called a Realization (although to me that only makes sense if there is something inherent in the data structure that ensures its contents realize some specific constraings), then the gadget returned by having the engine try to realize() could be named something altogether different, like "EngineRun" or "EngineData."
I guess what I am mostly saying is to choose quite distinct stem words for the two structures, to reflect the fact that this is a containment relationship, rather than an is-a or flavor-of relationship: an output-of-realize gadget optionally contains one of the collection-of-coordinates things, but it is an altogether different entity.
Hopefully I am making sense.
That makes the most sense to me! The structures would become something like this:
During our meeting, we decided to name the structures like this:
In the future, we might want the engine to return a best guess even if realization fails. Ways to do that include:
configinto aConfigNeighborhoodand a success indicator.configto something like this: TheConfigNeighborhoodin theErrvariant would be the best guess.Two thoughts:
In the new Realization structure, the ConfigNeighborhood member could be called "outcome" or "output" or something -- it's name needn't just reiterate its type.
In the possible "best guess" future world, we may still want to allow the engine sometimes to throw up its hands completely. So that would mean the two ways you suggest would become having the outcome be an
Option<ConfigNeighboorhood>together with a success indicator, or having it be something likeResult<ConfigNeighboorhood, (Option<ConfigNeighborhood>, String). But either way (optional best guess or mandatory best guess), having a ConfigNeighborhood on both sides of the Result feels a bit redundant.How about keeping the name
result? This both hints that it has aResulttype and accurately describes that it's the main result of trying to realize (in contrast to the extra data inhistory). To me, it feels similar tooutcome, but a little better because it provides the type hint.Yup, result is fine by me.
Renaming done (commit
f1865f8). I decided not to add the type aliasConfigurationforDMatrix<f64>, as discussed belowThese names seem fine, so closing, but note I did raise one question about the name of a member of ConfigNeighborhood.
(in case it wasn't clear I've finished this round of review ;-)
Whoops: it looks like we can't use the new module filename system here, because when I put a file called
common.rsin theexamplesdirectory, Rust thinks it's an example! Should I try to find a workaround, or just keep using the old module filename system?The shared crate technique from this StackOverflow answer is one possible workaround, but I'd feel weird creating a whole crate for a few lines of common code.
Commit
2137284gives the script a.shextension and makes it non-executable, so it has to be run with a commandlike
sh run-examples.sh. This commit also removes the shebang line, which I'd expect to be irrelevant for non-executable scripts.@Vectornaut wrote in #92 (comment):
Umm I do not fully understand what is going on here, but yes, we need sane names (i.e. not "mod.rs" for a file that has logging functions). Why does the common code have to be in the examples directory at all? Or conversely, why does it need to be a "module", whatever exactly that is in Rust?
People who stare at Rust source trees all day will read the filename as
common/mod.rs, which translates as "the top level of thecommonmodule" in the old module filename system. Of course, knowing that doesn't make the name any more legible to anyone else.The Rust documentation suggests this way of sharing code among Cargo integration tests, and it also seems to be widely used for Cargo examples, which have the same directory structure. There are other ways to accomplish this, and some of them might allow more legible naming, but they have their own disadvantages, described at the end of this comment.
A review of modules
In the dyna3
srcdirectory, the filesadd_remove.rsassembly.rsdiagnostics.rsdisplay.rsengine.rsoutline.rsspecified.rseach define a module. These modules are declared by the expressions
mod add_removemod specifiedat the top of
main.rs. As you've probably picked up through code review, modules define namespaces, control code visibility, and help factor out shared code.Modules in integration tests and examples
When it comes to sharing code among Cargo examples, I see the following advantages to putting shared code in a submodule in the
examplesdirectory:include!macro, but the macro documentation explicitly discourages this usage; it recommends using modules instead.If you think the potential for better naming outweighs these advantages, I'll look more into the alternate approaches I've been comparing to.
At the last meeting, we agreed to put the common code for examples where it makes the most sense, and just do whatever's needed in the Rust syntax for it to locate the code where we've put it. In particular, the filename will be something like
...examples/common/print.rs. We also settled on names Configuration for a raw collection of coordinates for everything, ConfigurationNeighborhood for that plus a subspace of the infinitesimal motions at that position, and Realization for a Result containing that and an optimization history.@glen wrote in #92 (comment):
This has now been implemented in commit
477d6a5, with a commit message explaining why we're using the frowned-upon#[path]annotation.@glen wrote in #92 (comment):
Done in commit
f1865f8. I decided not to add the type aliasConfigurationforDMatrix<f64>, because the places where it seemed suitable (likeConfigurationNeighborhoodandSearchState) and the places where it didn't (likeConfigSubspaceandPartialMatrix) were closely intertwined, so it seemed confusing to use the alias in some places but not others.The pull request should now be ready for the next round of review!
Checked out the latest commit in this PR. Then from top-level-directory:
Is it possible to make run-examples.sh work independent of the current directory when it is executed?
@ -711,0 +714,4 @@self.descent_history.set(history);match result {Ok(ConfigNeighborhood { config, nbhd: tangent }) => {Is
nbhd: tangentjust a syntax for in essence renaming thenbhdmember of a ConfigNeighborhood totangentfor use as a local variable? Is the fact that you are inclined to do so an indication that perhapsconfigandtangentwould be better names for the members of a ConfigNeighborhood rather thanconfigandnbhd? In other words, you seem comfortable enough with config just to use it here, but felt motivated to use thenbhdunder the nametangent...Yes—this code destructures
resultinto local variables calledconfigandtangent, which hold the values ofresult.configandresult.nbhdrespectively.That field was indeed called
tangentbefore the naming improvement commit. I renamed it tonbhdbecause theConfigNeighborhoodstructure seems well-designed to represent any first-order neighborhood of a point in configuration space, regardless of whether we're thinking of it as the tangent space of a submanifold. To me, this aligns with our renaming of theConfigNeighborhoodstructure itself, which recognized that this structure is well-designed to represent things other than the result of a realization.I'd be open to switching to a name like
ConfigTangentorConfigTangentPlanefor the structure and going back totangentfor the field in that context. I still feel like there must be a better name for the structure, like maybe:However, I haven't been able to find one.
OK, just these last two questions/comments are all I see left for this PR at this point. Thanks!
@glen wrote in #92 (comment):
Yes, it should be! I'm not quite finished working out how, but I should be able to do it by Monday.
run-examples.shless location-dependent f8e9624fe3run-examples.sh@glen wrote in #92 (comment):
Commit
f8e9624accomplishes this, as long as you invoke the script by callingshor another interpreter rather than callingsouce. This approach makes the repeatedcargocall longer, so in commitedeb080I've factored out the repeated call.OK, works for me and looks fine. I get the feeling that you're not troubled by calling the tangent subspace nbhd in the struct and tangent in the place it's used (I'd pick one or the other and use it in both places, but it's not worth fussing over), so I am merging this. Thanks!