search

opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
758 stars 308 forks source link

Component Enhancements #3745

Closed fcanderson closed 3 months ago

fcanderson commented 3 months ago

Introduction

This PR contributes a number of enhancements to the OpenSim::Component class. There are two areas of focus: 1) Bringing Modeling Options & Discrete Variables Up To Speed, and 2) Adding Low-Level State Trajectory Infrastructure.

The motivation for the enhancements originated with my efforts to wrap my SimTK::ExponentialSpringForce contact model and bring it into OpenSim. SimTK::ExponentialSpringForce makes extensive use of discrete states. It has four: 1) static coefficient of friction [double], 2) kinetic coefficient of friction [double], 3) elastic anchor point [Vec3], and 4) sliding state [double]. While the coefficients of friction could be handled by using OpenSim properties or a control file, the elastic anchor point (ANCHOR) and sliding state (SLIDING) could not.

ANCHOR and SLIDING are not inputs to a simulation; they are outputs. They are implemented in Simbody as auto-update discrete states. Auto-update discrete states are like continuous states in that they are updated every time step during numerical integration. They are different in that their time evolution is governed, not by differential equations, but by algebraic expressions.

Simbody State Types:            Corresponding OpenSim Variables:
  continuous (q, u, z)            state variable (coordinate, speed, force, activation, ...)
  discrete                        discrete variable
  auto-update discrete            discrete variable
  modeling option                 modeling option

A number of issues arose for me as I worked to bring SimTK::ExponentialSpringForce's discrete states into OpenSim:

I first considered an ad hoc approach for capturing the time histories of ANCHOR and SLIDING (e.g., saving them to a file with a custom format). This approach seemed inelegant and without broad benefit however.

After meeting with @nickbianco , we decided that a better approach by far would be to make good on an existing OpenSim TODO by adding the ability to comprehensively de/serialize all SimTK::State variables (i.e., continuous states, discrete states, and modeling options) from/to a single XML file. See https://github.com/opensim-org/opensim-core/blob/main/OpenSim/Simulation/StatesTrajectory.h#L484. An XML format seemed best, as SimTK already possessed the ability to stream non-double numerical types. In addition, XML attributes would be an ideal way to encode variable type (double, Vec3, etc.) and identify states by name – falling in line with OpenSim’s existing Component infrastructure (i.e., finding a state via its component path).

Addressing the issues described above and following through on the comprehensive de/serialization plan resulted in the enhancements in this PR.

Going forward, assuming no significant concerns arise during review and this PR is merged, the plan is to submit a PR for class OpenSim::StatesDocument (a working implementation of state trajectory de/serialization) and then finally to submit a PR for class OpenSim::ExponentialContact (the wrapper for SimTK::ExponentialSpringForce).

Brief Summary of Changes

None of the enhancements in this PR require any of OpenSim’s existing code base (outside of class Component) to change. The enhancements are garnered either by making internal changes to a few existing methods without changing their arguments or by the addition of entirely new methods.

Focus 1: Bringing Modeling Options & Discrete Variables Up To Speed

Focus 2: Adding Low-Level State Trajectory Infrastructure

For computational efficiency and convenience, six low-level state trajectory methods were added to OpenSim::Component. They are templated to accommodate a range of numerical types. Collectively, they form the basis for performing a complete serialization and deserialization of a SimTK::State trajectory, as is commonly collected during a simulation (i.e., a SimTK::Array_<SimTK::State>), to and from a single file.

void getStateVariableTrajectory(const string& path, const Array_<State>& input, Array_<T>& output) const
void getDiscreteVariableTrajectory(const string& path, const Array_<State>& input, Array_<T>& output) const
void getModelingOptionTrajectory(const string& path, const Array_<State>& input, Array_<T>& output) const
void setStateVariableTrajectory(const string& path, const Array_<T>& input, Array_<State>& output) const
void setDiscreteVariableTrajectory(const string& path, const Array_<T>& input, Array_<State>& output) const
void setModelingOptionTrajectory(const string& path, const Array_<T>& input, Array_<State>& output) const

Testing Completed

I added the following Catch2 test cases to testComponentInterface.cpp.

TEST_CASE("Component Interface Component::resolveVariableNameAndOwner")
TEST_CASE("Component Interface Modeling Options")
TEST_CASE("Component Interface Discrete Variables")
TEST_CASE("Component Interface Discrete Variables Vec3")
TEST_CASE("Component Interface State Trajectories")

In addition, I’ve run fairly thorough tests with OpenSim::StatesDocument and OpenSim::ExponentalContact. These tests are not included with this PR; they'll be included with the PRs for OpenSim::StatesDocument and OpenSim::ExponentalContact.

All existing test cases and all new test cases in OpenSim pass for Release and RelWithDbgInfo builds.

CHANGELOG.md

This change is Reviewable

nickbianco commented 3 months ago

@fcanderson, great to see these changes coming in! Did you want a review now?

fcanderson commented 3 months ago

Hi @nickbianco! Yes -- the code is ready (I think). A review would be great.

fcanderson commented 3 months ago

Thank you for the review, Nick. I'll start making the revisions.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 69 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: consider `funcName` or `methodName` to match other arguments (for this exception class, and the others below).

done I changed all instances of the func argument in the Component Exceptions to methodName.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 930 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Authorship credit would be suited for the author list in license header at the top of the file (we don't typically do this for individual added methods).
fcanderson commented 3 months ago

OpenSim/Common/Component.h line 950 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Can remove.

done

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1486 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Remove.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1469 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Consider using the Doxygen macro `@note` here.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1622 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Remove (after moving authorship credit to header).

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1749 at r1 (raw file):

    *          const SimTK::Value<Vec3>& valVec3 =
    *              SimTK::Value<Vec3>::downcast(valAbstract);
    *          Vec3 x = valDbl + Vec3(0.4);

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1854 at r1 (raw file):

    *          SimTK::Value<Vec3>& valVec3 =
    *              SimTK::Value<Vec3>::updDowncast(valAbstract);
    *          valDbl = Vec3(0.4);

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1874 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Remove.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1858 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
If it exists:

This "See" is intended to be setDiscreteVariableValue<T>. Using updDiscreteVariableValue<T> requires that the caller handle the downcasting. setDiscreteVariableValue<T> takes care of the basic downcasting.

Perhaps I need to work on the comments to make the distinction between the upd and set clearer?

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1626 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: the method name suggests that this could be used for any variable, but its only for discrete variables and modeling options. I'm not sure how you would rename it without making it overly long though. (Unless this could also apply to continuous variables, etc.) One option could be to have separate methods: `resolveDiscreteVariableNameAndOwner` and `resolveModelingOptionNameAndOwner` (if it makes sense and doesn't require too much duplication).

Good point.

Looking at the code, I think resolveVariableNameAndOwner() will actually work for StateVariables. I just haven't tested it.

I will see if I can insert some tests in testComponentInterface.cpp to see if resolveVariableNameAndOwner() will work for StateVariables. If so, I will update the documentation to clarify that resolveVariableNameAndOwner() can be used for StateVariables, DiscreteVariables, and ModelingOptions.

If one method works for all three, seems like we can leave the name as is?

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1636 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: `StateVariable`s are also not `Component`s, but `Component` provides methods to traverse to them (e.g., `traverseToStateVariable`).

Right. In the comments, I don't actually say that StateVariables are Components. What I mean to say is...

"Like StateVariables, the usual component traversal methods cannot be used on ModelingOptions or DiscreteVariables, and therefore a dedicated traversal method is needed."

The method traverseToStateVariable() won't work for ModelingOptions and DiscreteVariables because a different call signature is needed. traverseToStateVariable() returns a pointer the StateVariable itself. On the other hand, resolveVariableNameAndOwner() returns a pointer to the Component owner AND the name of the variable (whether for a discrete variable or modeling option).

As I mentioned in the previous comment, I believe resolveVariableNameAndOwner() will work for all three variable types.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1626 at r1 (raw file):

Previously, fcanderson (Clay Anderson) wrote…
Good point. Looking at the code, I think `resolveVariableNameAndOwner()` will actually work for `StateVariable`s. I just haven't teste it. I will see if I can insert some tests in `testComponentInterface.cpp` to see if `resolveVariableNameAndOwner()` will work for `StateVariable`s. If so, I will update the documentation to clarify that `resolveVariableNameAndOwner()` can be used for `StateVariable`s, `DiscreteVariable`s, and `ModelingOption`s. If one method works for all three, seems like we can leave the name as is?

I added a fairly comprehensive set of checks in

TEST_CASE("Component Interface Component::resolveVariableNameAndOwner")

and verified that resolveVariableNameAndOwner() works for StateVariables too.

I revised the doxygen comment block for resolveVariableNameAndOwner() to reflect this.

Is it reasonable to leave the method name as is? Or, do you think something like

resolveVariableOrOptionNameAndOwner()

would be better?

Under the covers, modeling options are actually discrete variables. It's just that they are a special case in that their realization stage is always Stage::Model.

fcanderson commented 3 months ago

CHANGELOG.md line 39 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
It would be good to note that these methods now _require_ a path, rather than a name.

A path is not required actually. More than the name is only needed if the calling component is not the owner of the option or variable. When called with just the name, the calling Component is assumed to be the owner (just as was the case for accessors before this PR).

I will try to clarify in the Doxygen comments that calling the accessors with just the name is fine.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1626 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Nice test addition and great that it works for `StateVariable`s. I think we can keep the name as-is then.

super. I'll leave the name as is then. Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 1636 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Thanks for clarifying. It could be helpful now to comment somewhere here that if you just need a `StateVariable` that `traverseToStateVariable` is available, so users don't use this to retrieve one in a roundabout way.

Good idea. On it.

fcanderson commented 3 months ago

Great! Thanks for the time you've spent on this, Nick! I'm responding to your comments now. In a few cases, I have questions for you, but for the most part I'm following through on your suggestions.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2387 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: more of a personal preference thing, but extra lines separating functions to me are unnecessary (and sometimes add visual clutter if used excessively).

Clarification: Do you mean the lines with "//__" preceding the methods?

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2385 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: To separate code sections, consider using the following style commonly used elsewhere in the codebase: ``` //============================================================================== /// GET TRAJECTORIES //============================================================================== ```

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2387 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Yes, this is what I was referring to.

Done. removed

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2421 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Something like "...this characteristic is not generaly true for discrete varaibles and therefore is not checked..." might be a bit clearer.

Why would discrete variables not generally be time ordered?

All the Component trajectory methods take a state trajectory in the form of Array_<SimTK::State>, which are generally gathered by a reporter during a simulation. Any Array_<SimTK::State> gathered during a simulation would be time ordered. Since discrete variables are part of the SimTK::State, just like the continuous states, the discrete variables would be time ordered too.

I can see how there may be quite a few discrete variables that are constant, but being constant doesn't mean that the values are not time-ordered. A continuous state could be constant (e.g., perhaps a coordinate that is locked) for a portion of a simulation, for example, but would still be time-ordered.

How are you seeing discrete states and modeling options as being different from continuous states?

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2421 at r2 (raw file):

Previously, fcanderson (Clay Anderson) wrote…
Why would discrete variables not generally be time ordered? All the Component trajectory methods take a state trajectory in the form of `Array_`, which are generally gathered by a reporter during a simulation. Any Array_ gathered during a simulation would be time ordered. Since discrete variables are part of the SimTK::State, just like the continuous states, the discrete variables would be time ordered too. I can see how there may be quite a few discrete variables that are constant, but being constant doesn't mean that the values are not time-ordered. A continuous state could be constant (e.g., perhaps a coordinate that is locked) for a portion of a simulation, for example, but would still be time-ordered. How are you seeing discrete states and modeling options as being different from continuous states?

I've had time to think about your comment a bit more. Maybe you just mean "constant" as opposed to "time-varying"?

It seems like there are a enough examples of time-varying discrete variables (e.g., control inputs [muscle excitations, motor throttle, etc.], coefficients of friction for a surface that has slippery patches, the elastic anchor point for my contact model, etc.) that not checking on the grounds that discrete variables or modeling options are mostly constant doesn't seem like a solid rationale for not checking the time-ordered-ness.

I feel more comfortable saying: it is the user's responsibility to check time-order-ness if that characteristic is important for the particular use-case.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2516 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Why is the subsystem not available from the `Component` in this case?

Only the allocator of the modeling option knows from which subsystem an option was allocated. Currently, there's no mechanism to import externally-allocated modeling options into OpenSim.

Right now, OpenSim assumes that all modeling options listed in its std::map for modeling options (see line 2521) are allocated from the SimTK::DefaultSubsystem. This may not be the case if modeling option was allocated externally by a Simbody class, for example.

This is the same issue I encountered for discrete variables. For discrete variables, I needed to implement a mechanism to 1) prevent double allocation, 2) initialize the discrete variable index, and 3) initialize the associated subsystem.

To handle modeling options more generally in OpenSim, the same or similar thing would need to be done for modeling options as I did for discrete variables.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2535 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Use section header style suggested above.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3233 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Trying to suggest something more descriptive, rather than indicating that a change occured:

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3286 at r2 (raw file):

    * the subsystem from which it was allocated. The initialization should be
    * done by implementing an overridding extendRealizeTopology() method in the
    * wrapper code and, in that method, calling this method. See class

Done. I added the word "overriding" to emphasis that this call cannot be made from the base Component::extendRealizeTopology(). This would be apparent to an experienced programmer, but I'm not sure it will be understood immediately by a less experienced programmer.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3791 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Rather than saying "Added to support..." at the top at the comment, consider adding something to end of the comment along the lines of "This may be useful when de/serializing discrete variables...".

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3801 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Same comment as above.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 4067 at r2 (raw file):


        // Introduced two data members: 'subsystem' and 'allocate'.
        // These data members allow OpenSim::Component to expose a discrete

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 2516 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
I see now, for discrete variables, the subsystem is stored in the `DiscreteVariableInfo`. So my actual question is, why not store the subsystem for modeling options in `ModelingOptionInfo` as well? I would advocate for this change to keep things consistent (unless there something that makes this difficult).

Nothing makes this difficult. I just didn't want to make any more changes than necessary.

I'm on the same page as you. It would be best to implement the same mechanic for modeling options as for discrete variables. Doing so would allow the "TODO" to be removed. And then, the way would be clear for a Simbody class that allocates modeling options externally to be wrapped in OpenSim as well.

I will go ahead and implement the same mechanic for modeling options as for discrete variables.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 4074 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Consider using`SimTK::SubsystemIndex` and accessing the subsystem when needed instead of carrying a pointer to the subsytem around in this struct.

What are the advantages of using the SimTK::SubsytemIndex instead of the pointer to the subsystem?

Having the wrong SubsystemIndex would be just as catastrophic as the wrong subsytem pointer. Nothing would crash; you'd just get wrong numbers.

Storing the SubsystemIndex would require a little more code every time you needed to acquire the pointer. In addition, we couldn't use nullptr as a clear signal that the SimTK::DefaultSubsystem should be used. I suppose we could use an index of -1 to mean no index had been assigned.

Just want to get this part straight before implementing the mechanic for modeling options.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 4084 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Consider my suggestion above to rephrase this sentence.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 4074 at r2 (raw file):

Previously, fcanderson (Clay Anderson) wrote…
I've messed around with the idea of using the subsystem index instead of the pointer. It's looking like the code could be quite simple. I just have to iron out one wrinkle. Ideally, it would be good to initialize the subsystem index in `DiscreteVariableInfo` to the index for the SimTK::DefaultSubsystem. This is not possible in the definition of `DiscreteVariableInfo`, but it would be possible to set the subsystem index in`addDiscreteVariable()`. This would mean a more invasive changes to the original code, but I do think the changes would be good.

I successfully switched to using the subsystem index instead of using the subsystem pointer. The code turned out to be simpler and is working fine. So, good recommendation on your part! I believe I've adjusted the comments to go along with this switch.

Before I bring the ModelingOptionInfo struct up to speed with the DisctreteVariableInfo struct, I'd like to make sure you are good with the changes I made.

fcanderson commented 3 months ago

OpenSim/Common/Component.cpp line 601 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Comments like these in the .cpp file won't appear in Doxygen, and ff they don't add anything new to the comments in the header file, then I would suggest removing them. (Same goes for the other comments below).

Done. I removed this comment and others like it. Pretty much all the comments just repeated what is explained by comments in the .h file.

fcanderson commented 3 months ago

OpenSim/Common/Test/testComponentInterface.cpp line 420 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
As suggested above, using `SimTK::SubsystemIndex` would be cleaner, if possible.

Done.

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3295 at r6 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
To avoid changing the API, it would be better to add a new method that returns only the `SimTK::SubsystemIndex`, especially since you're just using the `DiscreteVariableInfo`s internally anyway. (Another option is to have a method that returns the `DiscreteVariableInfo` which has both pieces of information, but I'm not sure that's the best solution and maybe not scripting-friendly).

I weighed changing the API. A search revealed that this method is not used -anywhere- in OpenSim, not even in Component.h or Component.cpp. And, all opensim-core code compiled and all tests passed after this API change.

The issue with making two methods is that the string-based map search would be done twice when it only needs to be done once to get both indexes. For computational reasons, at this low level especially, it seems important to reduce the number string-based searches.

Returning the DiscreteVariableInfo struct I think is a better solution than having to methods, but still represents an API change.

How about ... Keep the original method as it was and label it deprecated. And add getDiscreteVariableIndexes() as the recommended API?

fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3295 at r6 (raw file):

Previously, fcanderson (Clay Anderson) wrote…
I weighed changing the API. A search revealed that this method is not used -anywhere- in OpenSim, not even in Component.h or Component.cpp. And, all opensim-core code compiled and all tests passed after this API change. The issue with making two methods is that the string-based map search would be done twice when it only needs to be done once to get both indexes. For computational reasons, at this low level especially, it seems important to reduce the number string-based searches. Returning the DiscreteVariableInfo struct I think is a better solution than having to methods, but still represents an API change. How about ... Keep the original method as it was and label it deprecated. And add `getDiscreteVariableIndexes()` as the recommended API?

To avoid returning an std::pair, the signature of the new method could instead be

void getDiscreteVariableIndexes(const std::string& name, SimTK::SubsystemIndex& ssIndex, SimTK::DiscreteVariableIndex& dvIndex);
fcanderson commented 3 months ago

OpenSim/Common/Component.h line 3295 at r6 (raw file):

Previously, fcanderson (Clay Anderson) wrote…
To avoid returning an std::pair, the signature of the new method could instead be ``` void getDiscreteVariableIndexes(const std::string& name, SimTK::SubsystemIndex& ssIndex, SimTK::DiscreteVariableIndex& dvIndex); ```

This would probably easier for someone to use.

fcanderson commented 3 months ago

OpenSim/Tools/CMC.cpp line 1101 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
I recently changed how `Controller`s manage their list of `Actuator`s (now uses a list `Socket`). If you think this assert is failing because of that, let me know and I can investigate.

Yes. I believe the compile error is related. In the SimTK_ASSERT() there is a call to getActuatorSet(), which appears is no longer available. If the getSize() check is still important to do, it seems like getActuatorSet() should be replaced with the newer approach.

By the way, there are many asserts that fail when a Debug compile is performed. I imagine that folks know about this.

fcanderson commented 3 months ago

OpenSim/Tools/CMC.cpp line 1101 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
Replace `getActuatorSet().getSize()` with `getNumActuators()` should solve the issue here. I'm not aware of these failing asserts (I should get in the habit of creating debug builds more often!) -- are any others related to the `Controller` changes?

Done. Using getNumActuators() fixed the problem.

Once we finish this PR, I'll have another look at the assert failures and let you know what I see. There are maybe 6 to 10 unit tests that fail for a Debug build, if my memory serves me right. I didn't pay attention to which ones could be related to the Controller changes.

fcanderson commented 3 months ago

OpenSim/Common/Test/testComponentInterface.cpp line 1622 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…
nit: there is a lot of repitition here. Not sure if a helper function would reduce the number of lines needed, but something to consider.

A helper function would allow about 60 lines of repetitive code to be removed, but the helper function would need to be about 15 lines longer (you'd have to do some std::string concatenations), for a net gain of about 45 lines removed. But the code would probably be more difficult to read. I'm inclined to leave the code as is, but could be convinced otherwise.

fcanderson commented 3 months ago

Thanks @nickbianco for the excellent short story. I agree it would be good to get @aymanhab's take on this.

I would add ...

fcanderson commented 3 months ago

Woo hoo! It has taken some time. It is good to see this code merged!