Manage Controller actuators with a list Socket

[nickbianco]

Fixes issue #3675

Brief summary of changes

This PR updates how Controller maintains a list of controlled actuators by replacing the internally-managed Set<Actuator> with a list Socket. This mitigates the need for custom actuator connection code and a custom copy constructor and assignment operator to avoid memory leaks. In addition, all Controllers can now leverage other Sockets (including Inputs) in concrete implementations.

I tried to keep API changes minimal, but the switch to Sockets fundamentally changes Actuators connection, so some changes were necessary. A brief list of the main changes:

• The actuators_list list Property was removed in favor of the new list Socket called actuators. Therefore, setting any actuator connections via set_actuator_list(int) is no longer supported.
• Deprecated prescribing controls by index in PrescribedController
• PrescribedController got a full overhaul, including new code to support loading old models from XML.

After going through the tests and scripts, these changes seemed to be pretty reasonable. Most usages of PrescribedController didn't set controls by index, but rather by actuator name, which is the interface being kept here.

Testing I've completed

• Add Controller interface tests to testControllers.cpp.
• Updated other tests with necessary API changes.

Looking for feedback on...

testCMCArm26_Thelen.cpp is failing for reasons I've yet to identify. It fails when comparing muscle activations against the current standard file, for only a single muscle. I'm not sure why this single test fails when all other CMC tests pass. I noticed that testCMCArm26_Millard.cpp passes, but uses looser tolerances for the muscle activation comparisons. Is it possible that testCMCArm26_Thelen.cpp is just fragile and some small differences in Actuator computations in the CMC controller from this PR are leading to slightly different results? CMC uses some dark magic rather odd memory managment of Controllers, so maybe the switch to Sockets is having a subtle effect on how Actuator controls are computed. Or maybe I fixed a bug, and the old standard file is now invalid 🤷.

CHANGELOG.md (choose one)

• updated.

---

This change is 

[aymanhab]

Reviewed 4 of 29 files at r1, all commit messages.
Reviewable status: 4 of 29 files reviewed, 5 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Simulation/Control/Controller.h line 128 at r1 (raw file):

    /** Replace the current set of actuators with the provided set. */
    void setActuators(const Set<Actuator>& actuators);

Is this interface still in use? How do clients create the set?

---

OpenSim/Simulation/Control/Controller.h line 130 at r1 (raw file):

    void setActuators(const Set<Actuator>& actuators);
    void setActuators(
        const SimTK::Array_<SimTK::ReferencePtr<const Actuator>>& actuators);

Check my comment below regarding the use of ReferencePtrs across the interface

---

OpenSim/Simulation/Control/Controller.h line 181 at r1 (raw file):

    // to the list Socket.
    std::vector<std::string> _actuatorNamesFromXML;

If these names are only intended for xml upgrade, then they should probably have a life time that lasts only during the conversion rather than live forever with the Controller if possible. In theory those names would be re-inserted under the Controller's XML but formatted as sockets.

---

OpenSim/Simulation/Control/Controller.cpp line 58 at r1 (raw file):

    setAuthors("Ajay Seth, Frank Anderson, Chand John, Samuel Hamner");
    constructProperty_enabled(true);
}

Great to see this memory owner business retired 👍

---

OpenSim/Simulation/Control/Controller.cpp line 174 at r1 (raw file):

}

SimTK::Array_<SimTK::ReferencePtr<const Actuator>>

My recollection is that ReferencePtrs didn't play nicely with scripting, since the actuator list is fixed post model initialization, can we use regular const References instead across the interface?

[nickbianco]

Reviewable status: 3 of 29 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

OpenSim/Simulation/Control/Controller.h line 128 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Is this interface still in use? How do clients create the set?

Model still has getActuators(), which returns a Set<Actuator>. This is mostly here for backwards compatbility. The difference now is that the Set<Actuator> is only used to make the Socket connections, and not any suspect memory managemet stuff.

---

OpenSim/Simulation/Control/Controller.h line 130 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Check my comment below regarding the use of ReferencePtrs across the interface

Done.

---

OpenSim/Simulation/Control/Controller.h line 181 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

If these names are only intended for xml upgrade, then they should probably have a life time that lasts only during the conversion rather than live forever with the Controller if possible. In theory those names would be re-inserted under the Controller's XML but formatted as sockets.

I agree, but this is the only solution I could find to make old XML files backwards compatible. I use it to strip the actuator names from the XML in updateFromXMLNode, and then use these names to look up actuators in extendConnectToModel. In other words, I needed a way to pass information between updateFromXMLNode and extendConnectToModel, and a member variable seemed like a reasonable (if not ideal) approach.

If there's a better approach I'm not seeing, I'd be happy to make a change.

---

OpenSim/Simulation/Control/Controller.cpp line 58 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Great to see this memory owner business retired 👍

Done.

---

OpenSim/Simulation/Control/Controller.cpp line 174 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

My recollection is that ReferencePtrs didn't play nicely with scripting, since the actuator list is fixed post model initialization, can we use regular const References instead across the interface?

This actually isn't really necessary. I originally had it for a solution in CMCTool, but I needed a different approach anyway. Removed.

[nickbianco]

@aymanhab, after looking into it again, I remember why I needed _actuatorNamesFromXML. In order to avoid this extra member variable, I'd need to implement extendFinalizeConnections() to intercept the legacy actuator names before the Socket tries to make connections based on them. However, ModelComponent does not provide an interface to extendFinalizeConnections() (it is marked final), therefore my only option is extendConnectToModel(). Unfortunately, extendConnectToModel() is called after extendFinalizeConnections(), so I need to temporarily store the legacy actuator names to avoid the component trying to connect to an invalid connectee path.

If there's another way around this (short of changing the ModelComponent API), I'm still open to alternatives.

[aymanhab]

Sorry for being a pain @nickbianco but can you post or send the xml for the Controller before and after conversion? All I'm saying is that if the after-conversion can be created from the pre-conversion XML then you don't have to do anything special or mess around with other component API, this is a purely XML transform, let the downstream code handle it as usual. If that's not possible let me know and I'll move on from this sticking issue. Thanks

Reviewed 4 of 29 files at r1, 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 9 of 29 files reviewed, 3 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

    forward.updateModelForces(*model, "");

    // This initSystem() call needs to happen after updateModelForces() and

This may break other user code unrelated to Controllers?

---

OpenSim/Simulation/Control/Controller.h line 128 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Model still has getActuators(), which returns a Set<Actuator>. This is mostly here for backwards compatbility. The difference now is that the Set<Actuator> is only used to make the Socket connections, and not any suspect memory managemet stuff.

👍

---

OpenSim/Simulation/Control/Controller.h line 181 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I agree, but this is the only solution I could find to make old XML files backwards compatible. I use it to strip the actuator names from the XML in updateFromXMLNode, and then use these names to look up actuators in extendConnectToModel. In other words, I needed a way to pass information between updateFromXMLNode and extendConnectToModel, and a member variable seemed like a reasonable (if not ideal) approach.

If there's a better approach I'm not seeing, I'd be happy to make a change.

Thanks for the explanation @nickbianco. What I'm suggesting is using the XML DOM itself to preserve the names e.g. <actuator_list>act1 </actuator_list> becomes something like <Socket_list>/forceset/act1</Socket_list>, as long as the resulting XML you inject into the DOM is on the new format, you shouldn't need to do anything different downstream in extendConnect... but I could be missing something here that's specific to the Controllers and/or Sockets handling. If you diff the xml before and after in your environment you'll likely see the target syntax from there it's messy but localized change.

[nickbianco]

@aymanhab, no worries happy to clarify. The biggest change is that connected actuators will now have their whole path stored in the XML.

Before this change, the XML, for example, looks like this:

<ControlSetController name="Control_Set_Controller">
  <!--A list of actuators that this controller will control.The keyword ALL indicates the controller will controll all the acuators in the model-->
  <actuator_list>actu_name1 actu_name2</actuator_list>
  <!--XML file containing the controls for the controlSet.-->
  <controls_file> subject01_walk1_controls.xml </controls_file>
</ControlSetController>

And after this change:

<ControlSetController name="Control_Set_Controller">
  <!--A list of actuators that this controller will control.The keyword ALL indicates the controller will controll all the acuators in the model-->
  <socket_actuators>/path/to/actu_name1 /path/to/actu_name2</socket_actuators>
  <!--XML file containing the controls for the controlSet.-->
  <controls_file> subject01_walk1_controls.xml </controls_file>
</ControlSetController>

We could try prepending all the actuator names with /forceset, but this won't work if someone has an actuator not stored in the ForceSet, e.g., top-level path /actu_name1. The only way I've found (so far) to rigorously check the paths is my current implementation.

[nickbianco]

Reviewable status: 9 of 29 files reviewed, 3 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

This may break other user code unrelated to Controllers?

I think this is specific to how updateModelForces() modifies the model. The connections need to be resolved (via initSystem()) before passing to the ForwardTool.

---

OpenSim/Simulation/Control/Controller.h line 128 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

👍

Done.

---

OpenSim/Simulation/Control/Controller.h line 181 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Thanks for the explanation @nickbianco. What I'm suggesting is using the XML DOM itself to preserve the names e.g. <actuator_list>act1 </actuator_list> becomes something like <Socket_list>/forceset/act1</Socket_list>, as long as the resulting XML you inject into the DOM is on the new format, you shouldn't need to do anything different downstream in extendConnect... but I could be missing something here that's specific to the Controllers and/or Sockets handling. If you diff the xml before and after in your environment you'll likely see the target syntax from there it's messy but localized change.

See my comment on the main discussion thread.

[carmichaelong]

all minor things from my end. generally looks great, and a lot of the changes with how memory is being managed looks much better

Reviewed 23 of 29 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @aymanhab and @nickbianco)

---

Applications/Analyze/test/testAnalyzeTool.cpp line 492 at r2 (raw file):

        controller->addActuator(
                pendulum.getComponent<CoordinateActuator>("/tau1"));
        controller->prescribeControlForActuator("tau0", new Constant(0.0));

remove this line? updated a few lines down. is there a reason to check that that works here?

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think this is specific to how updateModelForces() modifies the model. The connections need to be resolved (via initSystem()) before passing to the ForwardTool.

i don't think i know how often or when updateModelForces() is used, so can't tell how big of a deal this might become for users that a certain flow for setting a model for a tool now is broken. if there could possibly be other use cases, it could be noted in the changelog generally that in some cases, initSystem() now needs to be called before passing the model to a tool

---

OpenSim/Simulation/Control/Controller.cpp line 38 at r2 (raw file):

//=============================================================================
// CONSTRUCTOR(S) AND DESTRUCTOR

🤩

---

OpenSim/Simulation/Control/ControlSetController.cpp line 272 at r2 (raw file):

    for (int i = 0; _controlSet != nullptr && i < _controlSet->getSize(); ++i) {
        std::string actName = _controlSet->get(i).getName();
        std::cout << "actName = " << actName << std::endl;

temporary debug printing?

---

OpenSim/Simulation/Control/PrescribedController.h line 12 at r2 (raw file):

 * through the Warrior Web program.                                           *
 *                                                                            *
 * Copyright (c) 2005-2023 Stanford University and the Authors                *

2024

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

    [[deprecated("this method no longer does anything")]]
    void prescribeControlForActuator(int index, Function* prescribedFunction) {}

1. does deprecated typically mean that the functionally is still there but not maintained?
2. any reason to keep this around for speed? seems like for all use cases i can think of, this should just be done once before any given simulation and be negligible to the rest of it

---

OpenSim/Tests/Wrapping/testWrapping.cpp line 88 at r2 (raw file):

        actuController.setActuators(actuatorsSet);
        const auto& socket = actuController.getSocket<Actuator>("actuators");
        for (int i = 0; i < (int)socket.getNumConnectees(); i++) {

pre-increment i?

---

OpenSim/Tools/CMCTool.cpp line 1106 at r3 (raw file):

/* Get the Set of model actuators for CMC that exclude user specified Actuators */
std::vector<std::string> CMCTool::getActuatorsForCMC(

consider changing the function name to getActuatorPathsForCMC?

---

OpenSim/Tools/CMCTool.cpp line 1109 at r3 (raw file):

        const Array<std::string>& actuatorsByNameOrGroup) {

    const Set<Actuator>& actuatorsForCMC = _model->getActuators();

note for myself: change here that this is no longer removing from actuatorsForCMC, but building up a list of actuators later (which i generally prefer).

doesn't seem like CMC is doing something weird because of this, so this should be the same

---

OpenSim/Tools/Test/testControllers.cpp line 10 at r3 (raw file):

 * through the Warrior Web program.                                           *
 *                                                                            *
 * Copyright (c) 2005-2023 Stanford University and the Authors                *

2024

[nickbianco]

Thanks @carmichaelong! Made some changes based on your comments, ready for review again.

Reviewable status: 21 of 30 files reviewed, 12 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

Applications/Analyze/test/testAnalyzeTool.cpp line 492 at r2 (raw file):

Previously, carmichaelong wrote…

remove this line? updated a few lines down. is there a reason to check that that works here?

Done.

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, carmichaelong wrote…

i don't think i know how often or when updateModelForces() is used, so can't tell how big of a deal this might become for users that a certain flow for setting a model for a tool now is broken. if there could possibly be other use cases, it could be noted in the changelog generally that in some cases, initSystem() now needs to be called before passing the model to a tool

updateModelForces() calls ForceSet::append() to append forces to the current model from a ForceSet loaded from a file. When I revert back to the original order of the setModel() and initSystem(), the test fails because the Socket component and the Model do not have the same root segment.

So I think the forces from file are being added via updateModelForces(), and then the new actuators are getting connected to the ControlSetController before the new model topology has been resolved. I suppose we could add an initSystem() call to the bottom of updateModelForces; this doesn't seem too unreasonable given that it is essentially a utility function anyway.

Giving this a try now, let me know what you guys think.

---

OpenSim/Simulation/Control/Controller.cpp line 38 at r2 (raw file):

Previously, carmichaelong wrote…

🤩

Done.

---

OpenSim/Simulation/Control/ControlSetController.cpp line 272 at r2 (raw file):

Previously, carmichaelong wrote…

temporary debug printing?

Done.

---

OpenSim/Simulation/Control/PrescribedController.h line 12 at r2 (raw file):

Previously, carmichaelong wrote…

2024

Done.

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

Previously, carmichaelong wrote…

1. does deprecated typically mean that the functionally is still there but not maintained?
2. any reason to keep this around for speed? seems like for all use cases i can think of, this should just be done once before any given simulation and be negligible to the rest of it

1. Typically yes, the functionality remains for deprecated method, but in this case it was not possible to keep the method (I needed to be able to determine indices to Socket connectees based on either actuator name or path, and these indices do not always align with the ControlSet indices.
2. No, for the reason you state.

---

OpenSim/Tests/Wrapping/testWrapping.cpp line 88 at r2 (raw file):

Previously, carmichaelong wrote…

pre-increment i?

Done.

---

OpenSim/Tools/CMCTool.cpp line 1106 at r3 (raw file):

Previously, carmichaelong wrote…

consider changing the function name to getActuatorPathsForCMC?

Done.

---

OpenSim/Tools/CMCTool.cpp line 1109 at r3 (raw file):

Previously, carmichaelong wrote…

note for myself: change here that this is no longer removing from actuatorsForCMC, but building up a list of actuators later (which i generally prefer).

doesn't seem like CMC is doing something weird because of this, so this should be the same

Done.

---

OpenSim/Tools/Test/testControllers.cpp line 10 at r3 (raw file):

Previously, carmichaelong wrote…

2024

Done.

[aymanhab]

Thanks @nickbianco I totally agree that trying to keep the names around and resolving paths later is more robust in the cases where this happens (actuators not under the forceset) but for the price of indefinitely keeping a class member this maybe too high a price to pay, well unless we know of too many such models that we distribute or build. Alternatively we can make sure that the error message if the actuator is not found is descriptive enough and keep the class(es) without the extra temporary data-member. Your call.

Reviewed 2 of 9 files at r4, all commit messages.
Reviewable status: 23 of 30 files reviewed, 13 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

updateModelForces() calls ForceSet::append() to append forces to the current model from a ForceSet loaded from a file. When I revert back to the original order of the setModel() and initSystem(), the test fails because the Socket component and the Model do not have the same root segment.

So I think the forces from file are being added via updateModelForces(), and then the new actuators are getting connected to the ControlSetController before the new model topology has been resolved. I suppose we could add an initSystem() call to the bottom of updateModelForces; this doesn't seem too unreasonable given that it is essentially a utility function anyway.

Giving this a try now, let me know what you guys think.

@nickbianco The method is called by the GUI side of the various tools. Will need to check if the order change causes a problem (for models with controllers?) but there could be user code modeled after our existing c++ code that breaks. Ideally we avoid that but alternatively we should document and test that our own GUI/tools/scripts work.

---

OpenSim/Simulation/Control/ControlSetController.cpp line 173 at r4 (raw file):

    for(int i = 0; i < na; ++i){
        const auto& actu = socket.getConnectee(i);
        actName = actu.getName();

Can we explain/document the rules for use of names vs paths? If we're moving to full paths, implicitly allowing for duplicate names but different paths, would this break the code below?

[nickbianco]

Alternatively we can make sure that the error message if the actuator is not found is descriptive enough

This will be difficult, since the error will occur during finalizeConnections(), and ModelComponents cannot extendFinalizeConnections(), so we won't be able to catch it. (If we could extendFinalizeConnections(), then this wouldn't be an issue at all.)

If we don't include this backwards compatibility then I'm inclined to convert all relevant models in the test suite/distribution and advertise this as a breaking change for older models. It will be too confusing when actuators in the ForceSet work fine but others don't because we silently are modifying the input.

Reviewable status: 23 of 30 files reviewed, 13 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

@nickbianco The method is called by the GUI side of the various tools. Will need to check if the order change causes a problem (for models with controllers?) but there could be user code modeled after our existing c++ code that breaks. Ideally we avoid that but alternatively we should document and test that our own GUI/tools/scripts work.

Adding theinitSystem() call to updateModelForces() should be pretty safe, no? It avoid the need to have a specifically placed initSystem() call in this test.

---

OpenSim/Simulation/Control/ControlSetController.cpp line 173 at r4 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Can we explain/document the rules for use of names vs paths? If we're moving to full paths, implicitly allowing for duplicate names but different paths, would this break the code below?

My preference would be to use paths everywhere, but I tried to support existing functionality as much as possible which is why I'm checking for names here. If we updated to paths, then all the example files would have to be updated (control set file headers use actuator names). It's something that we should probably do eventually, maybe for the next major release.

[nickbianco]

In other news, adding initSystem() to updateModelForces() magically fixed testCMCArm26_Thelen 🤷.

Reviewable status: 23 of 30 files reviewed, 13 unresolved discussions (waiting on @aymanhab and @carmichaelong)

[aymanhab]

Considering that there's no reasonable way for users to upgrade their models short of editing complex XML files, breaking changes for the vast majority of users is not acceptable. The question is whether there exist such models around with actuators outside the corresponding forceset that justify the burden of injecting data member into the class, I believe you answered this already. With that I rest my case and will approve the PR. Thanks @nickbianco for going through this 🥇

Reviewed 7 of 9 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Adding theinitSystem() call to updateModelForces() should be pretty safe, no? It avoid the need to have a specifically placed initSystem() call in this test.

It should be reasonable, we may see extra messages due to missing vtp files etc. that are produced by initSystem but probably safer. Regardless we must still document this since there are side-effects to initSystem that we may not be aware of at the moment.

[carmichaelong]

Reviewed 5 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nickbianco)

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

1. Typically yes, the functionality remains for deprecated method, but in this case it was not possible to keep the method (I needed to be able to determine indices to Socket connectees based on either actuator name or path, and these indices do not always align with the ControlSet indices.
2. No, for the reason you state.

two suggestions for what to do here:

1. in the deprecated message string, add what function should be used instead
2. throw an error that notes what function should be used instead

[nickbianco]

Thanks @aymanhab and @carmichaelong! Ready for review again.

Reviewable status: 28 of 30 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

Applications/Forward/test/testForward.cpp line 245 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

It should be reasonable, we may see extra messages due to missing vtp files etc. that are produced by initSystem but probably safer. Regardless we must still document this since there are side-effects to initSystem that we may not be aware of at the moment.

Sounds good. I updated the CHANGELOG.md, let me know if we should document anywhere else.

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

Previously, carmichaelong wrote…

two suggestions for what to do here:

1. in the deprecated message string, add what function should be used instead
2. throw an error that notes what function should be used instead

Done.

[carmichaelong]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: 29 of 30 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Done.

Sorry, I meant to imply one or the other, but I suppose both doesn't hurt to do! Will this deprecated message show up on doxygen too? Also, sorry if I missed it, but would be good to note this in the changelog so that users will know how to update their code

[nickbianco]

Reviewable status: 29 of 30 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

OpenSim/Simulation/Control/PrescribedController.h line 109 at r2 (raw file):

Previously, carmichaelong wrote…

Sorry, I meant to imply one or the other, but I suppose both doesn't hurt to do! Will this deprecated message show up on doxygen too? Also, sorry if I missed it, but would be good to note this in the changelog so that users will know how to update their code

Ah yes, should have done that in the first place. Done.

[carmichaelong]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aymanhab)

[aymanhab]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickbianco)

[nickbianco]

Thanks @carmichaelong and @aymanhab!