Skip to content
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

BF-Systems: Design for Hierarchical Systems #671

Closed
15 tasks
laxmikantbaheti opened this issue Mar 17, 2023 · 7 comments · Fixed by #699
Closed
15 tasks

BF-Systems: Design for Hierarchical Systems #671

laxmikantbaheti opened this issue Mar 17, 2023 · 7 comments · Fixed by #699
Assignees
Labels
BF Basic Functions/Infrastructure enhancement New feature or request v1.2.0 Extension Hub

Comments

@laxmikantbaheti
Copy link
Contributor

laxmikantbaheti commented Mar 17, 2023

Description/Motivation
Extension of Systems class for complex systems (Cascaded, , Parallel, Hierarchical, etc.) with possibilities to add tasks with dependencies.
image
image

Task list

  • 1. Updates on the System Class:

    • Inherits from Task
    • New optional parameter p_t_step, this attribute shall be passed in the simulate reaction method
    • New method _run() that calls the simulate reaction method
    • More updates as shown in the class diagrams for methods for getting functions
  • 1. New class bf.systems.MultiSystem (or ComplexSystem)

    • Parent classes: bf.systems.System, bf.mt.Workflow
    • New Method add_system()
      ... calls internally the method add_task()
    • Method _simulate_reaction(): complex parallel/aysnchronous simulation of systems within
      The simulate reaction of the multisystem simulates each of the subsystems by an amount equal to t_step in each cycle. This t_step is the smallest latency of all the subsystems inside
    • Method setup(), sets up a new state ("MultiState") that is a set of all the states and action spaces of all the sub-systems within. Subsequently each subsystems fetches it's interesting state/action by mapping over the mult-state
    • New method run(), runs all the run methods on the subsystem tasks, which consequently calls the simulate reaction method of the subsystems.
  • 2. Suitable Howtos

  • 3. Updates on RTD

    • Documentation bf.systems
    • Class diagram bf.systems

Related issues
#664 #672 #396

@laxmikantbaheti laxmikantbaheti added the enhancement New feature or request label Mar 17, 2023
@detlefarend detlefarend added BF Basic Functions/Infrastructure v1.2.0 Extension Hub labels Mar 20, 2023
@detlefarend
Copy link
Member

detlefarend commented Mar 20, 2023

Hi @laxmikantbaheti, I concretized the issue based on our last discussion. I propose to avoid the term "complex" due to two reasons:

  • the association with complex numbers
  • "multi" is more MLPro style (MultiAgent, MultiPlayer)

@laxmikantbaheti
Copy link
Contributor Author

Hi @laxmikantbaheti, I concretized the issue based on our last discussion. I propose to avoid the term "complex" due to two reasons:

  • the association with complex numbers
  • "multi" is more MLPro style (MultiAgent, MultiPlayer)

Hi @detlefarend , thanks for making it more concrete.
I agree with you. "Multi" brings it to a more lower level term.

@detlefarend detlefarend added this to the Sprint 2023/03 milestone Mar 30, 2023
@detlefarend detlefarend changed the title BF-Systems: Design for Complex Systems BF-Systems: Design for Hierarchical Systems Mar 31, 2023
@laxmikantbaheti
Copy link
Contributor Author

laxmikantbaheti commented Apr 12, 2023

Hi @detlefarend , @steveyuwono , I have added the first draft for the class diagrams Hierarchical Systems in bf/systems/system_enhancements. Currently its a separate file named BF-Systmes-MultiSystems_class_diagram.drawio, after agreeing to the changes I will move them into the single file for BF-Systems.

Currently, I think there is no need for a new state object MultiState, instead I propose to have a SystemShared object that has a dictionary for each system and also performs the mapping task.

Please let me know your opinion. Thanks in advance!

@laxmikantbaheti laxmikantbaheti linked a pull request Apr 12, 2023 that will close this issue
2 tasks
@steveyuwono
Copy link
Collaborator

Hi @laxmikantbaheti , thank you for the updates. I have checked the class diagram. In my opinion, it looks good and I agree with the introduction of the SystemShared object which makes the MultiState object not necessary.

Is the setup_spaces intended to be a method to set up the multisystem by calling the add_system method? If yes, is it possible to combine both methods? It means, when we add_system, it is directly setting up and adjusting the multisystem at the same time.

@laxmikantbaheti
Copy link
Contributor Author

Hi @steveyuwono , thanks for the feedback.

I think the setup_spaces() method of the MultiSystem can be a predefined method and not a custom method. It is responsible to collect all the substates and setup the shared object with mappings and state dictionaries.

But, also, there needs to be a way to assign the action and state space of a multisystem. Like when considered a blackbox, it has an action and state space. Maybe a parameter for multisystem. I'll think of this and update you...

@detlefarend
Copy link
Member

Hi @laxmikantbaheti , I just took a look at the class diagram. It is a clean design! Here are some annotations from my side:

  • Class System: _id, set_id()
    During my last refactoring of persistence, I introduced a new class Id, that adds everything around a unique Id (attribute, methods) to a child class. Since Task is persistent, it inherits from Persistent that, in turn, inherits from Id. Long story short: you don't need to care for it - except in the constructor (as you already did).
  • Multi-States
    In bf.ml, we introduced the HyperParamDispatcher that behaves like a HyperParamTuple but references to HPs of models you can add by calling method add_hp_tuple(). We could provide something like that on a lower level (bf.math) for spaces. That makes it easier to deal with multiple states, actions, ... on higher levels. Eventually, it could replace class MultiState...
    However, to support multitasking, placing it in a shared object is a good idea.
  • Class MultiSystem
    • Method _simulate_reaction() is not optional and is to be implemented. (non-trivial as discussed)
    • Methods for plotting: are also not custom at this level. They need to trigger the related methods of the embedded systems or deal with a Multi-Mujoco-file, on the other hand...
  • Copy/Paste errors
    • In class MultiSystem, the method compute_success() is there twice. One of them should be compute_broken().
    • Class SystemShared, first and second yellow boxes on the right are identical

Hope that helps...

@laxmikantbaheti
Copy link
Contributor Author

laxmikantbaheti commented Apr 13, 2023

Hi @laxmikantbaheti , I just took a look at the class diagram. It is a clean design! Here are some annotations from my side:

  • Class System: _id, set_id()
    During my last refactoring of persistence, I introduced a new class Id, that adds everything around a unique Id (attribute, methods) to a child class. Since Task is persistent, it inherits from Persistent that, in turn, inherits from Id. Long story short: you don't need to care for it - except in the constructor (as you already did).

  • Multi-States
    In bf.ml, we introduced the HyperParamDispatcher that behaves like a HyperParamTuple but references to HPs of models you can add by calling method add_hp_tuple(). We could provide something like that on a lower level (bf.math) for spaces. That makes it easier to deal with multiple states, actions, ... on higher levels. Eventually, it could replace class MultiState...
    However, to support multitasking, placing it in a shared object is a good idea.

  • Class MultiSystem

    • Method _simulate_reaction() is not optional and is to be implemented. (non-trivial as discussed)
    • Methods for plotting: are also not custom at this level. They need to trigger the related methods of the embedded systems or deal with a Multi-Mujoco-file, on the other hand...
  • Copy/Paste errors

    • In class MultiSystem, the method compute_success() is there twice. One of them should be compute_broken().
    • Class SystemShared, first and second yellow boxes on the right are identical

Hope that helps...

Hi @detlefarend , thanks for the review, it was very helpful. Sorry, I will clean the file a bit.

  • MultiState:

    INITIALLY
    I actually had second thoughts. I came to a realization that, we might not need a new class MultiState. Instead the SystemShared class cares for the mapping and sharing of information among the sub-systems. The run method of each system updates it's own state and the SystemShared updates the related mappings. And this dictionary of all the states is passed as **p_kwargs in the run.

    NOW
    I took a look at the hyperparameter dispatcher. The concept of multistate is very similar. We can reuse this logic, at a lower level combining multiple spaces and values into a single Tuple/Element.

  • simulate_reaction(): here, I actually had some thought, providing a possibility for user to extend the public simulate reaction at MultiSystem level. However, that does not require a special mention on the class diagram and also doesn't help at the moment. Finally, I agree with you...

  • Plotting: Here I had given these custom methods for the same reasons as to work with a Global Mujoco System/Visualization... However, currently I am not into sync with rizky's development in MuJoCo. I will go through it once.

laxmikantbaheti added a commit that referenced this issue May 8, 2023
laxmikantbaheti added a commit that referenced this issue May 8, 2023
laxmikantbaheti added a commit that referenced this issue May 8, 2023
laxmikantbaheti added a commit that referenced this issue Jun 7, 2023
laxmikantbaheti added a commit that referenced this issue Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BF Basic Functions/Infrastructure enhancement New feature or request v1.2.0 Extension Hub
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants