Skip to content

[DevProp] Exclude any Component from underlying System #1382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aseth1
Copy link
Member

@aseth1 aseth1 commented Nov 8, 2016

Enclosed is a proposal for a new feature of the Component to enable it to be effectively excluded from the Model and its resulting System. I do not see the proposed changes as a replacement for disabling Forces and Constraints. Instead it proposes a new feature to correctly and unambiguously exclude the Component (and its subcomponents). I believe users were using disable because they had not other means of removing a Component temporarily other than editing XML directly. The intent of this proposal is to address that use case.

Any and all feedback is appreciated. Thanks!


This change is Reviewable

@chrisdembia
Copy link
Member

This will be a good feature. The proposed test cases sound great.

  1. Consider the following code:
Model m;
Component& foo = m.updComponent("foo");
foo.set_is_excluded(true);
m.finalizeFromProperties();
foo.set_is_excluded(false);
m.finalizeFromProperties();
Component& fooAgain = m.updComponent("foo");

Would that throw an exception? That is, will a pointer to a component be the same across calls to exclude/include it? Or would copies, etc. occur? Consider using the test case to ensure such code will work (without re-fetching the excluded subcomponent from the model using getExcludedComponentList(), etc.).

  1. What do you think about adding methods like (get|upd)ExcludedComponent(), so that it is not necessary to use the ComponentList?
  2. How should the excluded components appear in the GUI? Should the GUI present components as they exist in XML? Or should the GUI present the excluded components as a separate tree? And if so, how would such a tree be organized, considering that the excluded components could be anywhere in the tree? Would it be a flat list with full paths? I imagine it is preferred that the GUI shows only 1 tree of all components. If so, then it is probably necessary for the API to give access to a list of all components, included excluded components. That is, a getIncludedAndExcludedComponents().
  3. What is the word we will use to describe non-excluded components? That is, if I ask you, "did you exclude the muscle component?" would you respond with "No, the muscle component is included."? Or "No, the muscle component is present." or "No, the muscle component is in-use."? Ideally, we would not say "No, the muscle component is not excluded."
  4. Related to the point above, I think we should at least explore terms that are "positive." For example, using "is_included" instead of "is_excluded" for the property. I currently do prefer "is_excluded" over "is_included", but we should think about this more. Double negatives are super confusing.
  5. I definitely think there is a strong use-case for the full excluding of subcomponents, but I also do not think it can be a full replacement for "is_disabled" (which I think is your stance as well, as you mentioned IAA). So I think we should expect the two sorts of flags to co-exist, and we should have clear documentation explaining the difference.
  6. My main concern with the is_excluded flag is that the resulting SimTK::System and SimTK::State are different. You will not be able to use the same State with two copies of the same model if one of them has a different value for one of the is_excluded flags. For example, imagine doing a simulation with two nearly identical models: they both have a ClutchedPathSpring, but model B has excluded the ClutchedPathSpring. Say you want to use the same SimTK::State for the initial state of the simulation of both models. This will not be possible, because the two models will have a different number of state variables. The solution for this specific issue is to use the is_disabled flag instead of is_excluded, but maybe we should think about this general problem a little.
  7. I think our convention is to use is_ or has_ before boolean properties (or at least for member functions). If we do have this convention, then I prefer is_excluded over excluded for the property name.

@aymanhab
Copy link
Member

aymanhab commented Nov 8, 2016

Thanks @aseth1 for the nice proposal. Few thoughts come to mind:

  1. While the Component/Tree layout gives the connotation that all Components are equal, the truth is that they aren't. For example, what does it mean to exclude a Coordinate of a Joint or a MuscleForceLength Component from a single Muscle? If there's no clear intuitive answer that users can assume/understand without reading oodles of documentation then these Components shouldn't be allowed to be "excluded".
  2. We should consider practical use cases for users creating or assembling models, and using "exclusion" to troubleshoot. We should discover what "actual" functionality or troubleshooting helpers that we can provide (initially at API and eventually the application) before we introduce yet another new concept.
  3. The co-presence of disabled and excluded will likely confuse users as you pointed out.
  4. What happens to "Connections, inputs, outputs" when a component is excluded. If the wiring is lost then very likely users will not do it, if not then all the connection/input/output wiring will need to handle the new excluded flag.
  5. A possible useful and less drastic option is to better implement the disabled flag and make it available to few more Component types that make sense (e.g. Forces, Constraints, Controllers, Reporters,..) and make sure disabled means do nothing (including no-side-effects). These probably can be easily implemented at a few base classes but you know that code better to make this call.
  6. From the GUI side, we currently assume fixed topology of the model once loaded, only few places where you can actually add new Components. Allowing "exclusion" means that Coordinate Window needs to be recreated repeatedly, same for plotter window (that shows Coordinates, Muscles), Navigator tree etc. Not impossible but is it justified?

@tkuchida
Copy link
Member

Related to #1111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants