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

Improving the attribute system #61

Closed
m-mirz opened this issue Dec 16, 2021 · 8 comments
Closed

Improving the attribute system #61

m-mirz opened this issue Dec 16, 2021 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@m-mirz
Copy link
Contributor

m-mirz commented Dec 16, 2021

Attributes should be declared as member variables of the component class to avoid something like attribute<Complex>("V_ref")->get() during the simulation. This is not efficient and the attributes of a component class are not easily deduced from looking at the component class declaration.
mInductance = addAttribute<Real>("L", Flags::read | Flags::write);

Each attribute should include

  • name
  • physical unit
  • fixed / variable, where variable is used for attributes that may be changed by the simulation

Accordingly, the python print should include more columns for this information.

Attribute list for object with name cs:
name                          type                size           value                         
-----------------------------------------------------------------------------------------------
I_ref                         Complex                            10.000000<0.000000            
f_src                         Real                               50.000000                     
i_intf                        MatrixReal          1x1            [...]                         
name                          String                             "cs"                          
right_vector                  MatrixReal          0x0            [...]                         
uid                           String                             "cs"                          
v_intf                        MatrixReal          1x1            [...] 

To make the work with the attributes more comfortable in the component classes, some operators like cast () and assignment operator might be overwritten but this should be limited as much as possible.

We also need to improve the mechanism for describing the output - input relation between two components.
Currently, it is possible to change the map value for a key calling something like this:
voltageSource->setAttributeRef("V_ref", attribute<Real>("V_ref"));
If you store are reference to an attribute inside a component like below, the stored reference might be invalid after calling the function above.
voltageRef = attribute<Complex>("V_ref");

We could add one member variable to the Attribute class that points to referenced Attributes, which describes an input output relation. Still, the value pointer address should be overwritten for the input Attribute to minimize the overhead when reading the value during simulation time.

@m-mirz m-mirz added the enhancement New feature or request label Dec 16, 2021
@JTS22
Copy link
Contributor

JTS22 commented Dec 16, 2021

As an initial idea, the attributes could be implemented by using two different levels of pointers. Every class that should hold attributes would have a member variable that stores a shared pointer to an attribute instance. At the same time, the underlying AttributeList manages a map that stores the attribute's ID and a shared pointer pointing to the same attribute instance. This way, every attribute of a class can be referenced both by the class' member variable as well as the entry in the AttributeList parent class.
Additionally, every attribute instance should hold a pointer to the actual value managed by this attribute. This decouples the value from the attribute's other variables like access rights or ID. Doing this should also allow for sharing a value pointer between two different attributes, which might be helpful for solving the reference issue.

When doing calculations involving the attributes, is should be possible to directly use the involved class' member variables (which would then be pointers to attributes with pointers to values). To make this as easy as possible (regarding syntax and computation effort), the dereference operator * probably has to be overloaded.

As a more advanced step, we might want to look at how the Task schedule is computed from the attributes' relations to each other. It might be possible to move the dependency information into the attributes themselves, specifying what other attributes an attribute depends on and how its value can be computed from its dependencies. While this is quite a large shift in how the scheduler manages dependencies, it might result in a simpler dependency graph and easier to understand scheduler code.

@JTS22
Copy link
Contributor

JTS22 commented Dec 20, 2021

Attribute<Real>::Ptr real() {
Attribute<Real>::Getter get = [this]() -> Real {
return this->getByValue().real();
};
Attribute<Real>::Setter set = [this](Real realPart) -> void {
Complex copyValue = this->getByValue();
this->set(Complex(realPart, copyValue.imag()));
};
return Attribute<Real>::make(nullptr, get, mFlags, shared_from_this());
//Real *realPart = &reinterpret_cast<Real*>(mValue)[0];
//return Attribute<Real>::make(realPart, mFlags, shared_from_this());
}

For the new attribute system, we might also want to look at how to implement these kinds of derived attributes in a different way. Currently, these create new attributes that reference the old attribute and compute their own values using Getter and Setter functions. In this example, this behavior is inconsistent as the generated setter is never used (instead nullptr is passed into the constructor, crashing the program if the setter is ever called). Also, when statically looking at the code, it appears as if the Getter function calls itself (via this->getByValue) which would lead to an infinite loop. This code only works because this is captured in the lambda clause at a point where the Getter (and the getter flag) are not set yet. While technically correct, this is rather confusing and hard to understand when reading the code.

@m-mirz
Copy link
Contributor Author

m-mirz commented Dec 26, 2021

The main application of the getters is to process data before sending it to VILLASweb and for logging. Often we send amplitude and phase to VILLASweb instead of real and imaginary value of a complex voltage. Setters were introduced to preprocess incoming data but eventually we never used them and just made sure that the incoming data is already compliant.

Since we built the attribute system, VILLASnode introduced hooks to transform relayed data. This covers many use cases already. For logging purposes, the getters are still convenient. I could imagine that having a clean solution for the getters would be sufficient since we can use VILLASnode for other use cases.

@JTS22
Copy link
Contributor

JTS22 commented Dec 29, 2021

All commits related to this issue will now be collected in PR #62. In the coming days I will post an overview on the (highly experimental) changes currently contained in the PR.

@JTS22
Copy link
Contributor

JTS22 commented Jan 1, 2022

Here are some thoughts on the current state of the attribute system as well as an explanation of the new system currently included in PR #62 :
Currently, to my knowledge, there are three different kinds of attributes, which are all instances of the same Attribute<T> class:

  • "static" attributes that own a value, which can be changed through the linked variable reference or through the get and set methods. This kind is the most common, used for example for the ID and Name of any identified object.
  • "derived" attributes that do not own a value, but compute it from the value of another attribute using Getter and Setter functions. These are created by the real and imag functions on complex attributes or by the coeff function for matrix attributes. A derived attribute can have the same or a different type compared to the attribute it's derived from.
  • "referencing" attributes. Currently these are not real attributes at all, but they are implemented by cloning the reference attribute and linking it to the attribute list of another object. Their purpose is to allow for one attribute to always have the same value as another attribute.

Additionally, dependencies can be defined for all of these attributes using the Scheduler and Task system. The various tasks describe which attributes need to be recalculated first when an attribute should be updated during the simulation.

The system introduced in PR #62 aims to unify these different types of attributes and dependencies, while allowing for full generality with regard to which attributes can be combined in which relationships. To do this, the new system provides two kinds of attributes which now have their own classes:

  • AttributeStatic<T>: This class describes an attribute which will always own its data value and can only be changed through direct get and set functions. A static attribute can never depend on any other attribute, it can only be used as a dependency.
  • AttributeDynamic<T>: While this attribute also contains a shared pointer to some data, this data should more be though of as some sort of value cache that might not accurately reflect the "true" value of the attribute. To compute this true value, a dynamic attribute can hold two lists of AttributeUpdateTasks. These tasks are invoked when either the get or set method of the dynamic attribute is called. Every task contains a list of dependency attributes as well as an actor function. This actor function receives the pointer to the dependent attribute's value cache as well as pointers to all the dependency attributes. Every actor function can therefore update the value cache (both its value as well as its pointer address) based on the values of the dependency attributes (which can be obtained via their respective get-methods. An actor function can also act in reverse, reading the cached value and updating the dependency attribute's via their set-methods. Currently, a mix of both actions would also be possible.
    When the get function is invoked on a dynamic attribute, it first calls all the actor functions registered for the get action. After these Tasks have potentially modified the cached data, a reference to this data is returned.
    For the set function, the cached value is immediately updated to the passed value. After this, all actor functions registered for the set action are called, which gives them the chance to propagate the update to any dependency attributes.

Using the dynamic attributes, the "derived" and "referencing" attributes from the old model can be recreated in a more general form:
"Derived" attributes can be represented by a dynamic attribute with one task each for the get and set action. The task for get computes the derived value from the parent attribute(s) and updates the cache, while the task for set can update the parent attribute(s) based on the new value of the derived attribute. For the real part of a complex attribute, these tasks can look something like this:

https://github.com/JTS22/dpsim/blob/98d9b471da7665144bace970bf8ba812fb23c03c/models/Include/cps/Attribute.h#L223-L230

"Referencing" attributes are implemented using a dynamic attribute with a task which updates not the cached data value, but the cache pointer address. When referencing a static attribute, it would suffice to update the cache pointer once at the beginning of the simulation. When referencing a dynamic attribute, this update has to be performed on every get operation as the data pointer of the referenced attribute could also be subject to change.

To allow for this scope of generality, the resulting attribute code gets quite complicated and template-heavy. To prevent the user from having to deal with these generic types, multiple derive functions are introduced that create new dynamic attributes from other (static or dynamic) attributes. Using C++ 20 concepts, these functions can be bound to requirements such that they are only available for the attributes with the right type. This means that, for example, the deriveReal and deriveImag functions are only available for attributes with complex type. For referencing attributes, a derive function might be introduced that correctly installs the get action for copying the data pointer, either once for static references, or on every get for dynamic references.

Lastly, I want to stress that most of these concepts currently just exist on paper and are therefore subject to change. For example, the high amount of templates might lead to an unacceptable increase in compiled code size. Or the additional function calls for management of the actor functions might cause too much of a performance overhead to actually be useful. The system might also be too general for the use case of DPsim, in which the special cases this new system allows for might never be needed.

@JTS22
Copy link
Contributor

JTS22 commented Feb 1, 2022

Since reworking the attributes entails reading through a lot of DPsim's current code, here are some random things I noticed that might be turned into new Issues later on:

  • The MNASolver uses different attributes depending on if it computes multiple frequencies in parallel (expressed through mFrequencyParallel)- The corresponding attributes mLeftSideVectorHarm and mLeftSideVector should probably be unified
  • Many of the basic components (Capacitor, Resistor, Inductance...) are (partly) derived from a base class which contains only the components main parameter (C, R, L...). Since these base classes are currently not derived from AttributeList, their attributes can not be initialized in the usual way, rather the initialization has to be moved into the subclasses.
  • The clone and setParameters methods are very similar for many basic components. Maybe a clone could just clone all attributes that have a certain "parameter" flag set
  • Why is Base::Ph1::Resistor::mConductance a thing? It is only used once in SP::Ph1::Resistor and is always the inverse of the resistance (as it should be?). If this is really necessary for some reason, we could make it a derived attribute.
  • For some DP and SP components (e.g. {DP,SP}::Ph3::VoltageSource), the component code is nearly identical. One might consider reusing the relevant computation tasks.
  • In the {DP,SP}::Ph1::NetworkInjection::daeInitialize methods, the Complex attribute "v_intf" is accessed, however, this is a complex matrix, therefore the program should crash if it ever reaches this point. Changing this to access the first element of the "v_intf"vector...
  • In all NetworkInjection classes, the attribute references are set in the constructor as well as in every setParameters method. As long as setParameters does not recreate the sub-VoltageSource, the first call to setReference in the constructor should be sufficient.
  • Some kind of formatter might really help make some of the calculation code more readable
  • Is there a rule to determine whether a method is implemented in the header or cpp file? For simpler methods (small setParameters, getters for attributes...) it seems kind of arbitrary
    • if not header only, it should be implemented in the cpp file by default
  • In DP::Ph1::PQLoadCS::initializeFromNodesAndTerminals the attribute mNomVoltage (which is a Pointer) is compared with Zero and then something is divided by the attributes value. There is likely a ->get() missing here...
    • Yes, the ->get() is missing
  • The DP::Ph1::PQLoadCS sets its right_vector attribute (which is declared in MNAInterface) to reference the right_vector of its sub-component. The right_vector should not be set to dynamic, so is there a better alternative here? This also happens in {DP, EMT}::Ph1::VoltageSourceRamp.
  • The Base::Ph1::PQLoad class is never used...
  • The DP::Ph1::ResIndSeries has two attributes mResistance and mConductance that are either never used at all (mConductance) or only set by setParameters, but not used in any calculations (mResistance)
    • if an attribute is never used > delete, if it is used somewhere but could be removed > mark with comment
  • Given the above points, a dead-code analysis for the whole project might be a good idea...
  • In the DP::Ph1::RxLine, the mVoltage member variable is only read once and never written (so its always zero), while the mCurrent variable is never used at all.
    • mark with comment
  • In the component classes, many member variables (especially regarding Conductance, Resistance, Capacitance, Inductance) are either never used at all (see above points) or only used once to initialize some sub-component. Many of these can probably be converted to local variables, leading to overall savings in memory usage.
    • mark with comment
  • The DP::Ph1::SynchronGeneratorTrStab has an option for setting a reference for Omega and Delta (mUseOmegaRef). This can probably be implemented better using the new dynamic attributes (see comments in the code).
  • The EMT::Ph3::SynchronGeneratorDQ defines a rather long list of attributes. We should check whether all of these are used within the subclasses
    • mark with comment
  • The {DP,EMT}::{Ph1,Ph3}::SynchronGeneratorIdeal defines an attribute V_ref which is set to reference the voltage of the sub-component's voltage-source. However, in the component code itself, V_ref is never used. Since there is no guarantee this is ever set correctly / readable from outside code, a getter method might be better here. This also seems to apply to other components where attributes solely exist for outside code to access sub-component attributes.
    • mark with FIXME
  • Every SimPowerComp<T> has a vector mSubComponents of sub-components but most classes derived from SimPowerComp have their sub-components as separate member-variables, not utilizing this list.
    • SImPowerCom list should be used, mark with FIXME
  • The DP_Ph1_Transformer.cpp and EMT_Ph3_Transformer.cpp are duplicated within the Source/SP folder
    • delete duplicates
  • The SP::Ph1::Load::updatePQ method references attributes P_nom and Q_nom that are never declared
    • mark with FIXME
  • The SP::Ph1::PiLine defines attributes that access individual Matrix sub-components. While this can be implemented using dynamic attributes, this seems kinda overkill. Especially since these attributes only seem to be used in one example and only for logging purposes (which has derive-methods already)
    • mark with FIXME
  • Some member variables from base classes (especially in Base::SynchronGenerator and Base::Ph1::Transformer) are used in some sub-classes as normal member variables and in some sub-classes as attributes. This is not doable using the new system, ever member has to be one or the other.
    • try to merge and mark
  • Some of the Signal-Components (e. g. FIRFilter, Integrator, PLL, PowerControllerVSI) rely on attributes being explicitly set by outside code. While possible, this requirement should be documented or facilitated through an explicit setter method.
    • add comment
  • There are components (Signal::Integrator, Signal::PLL, Signal:: PowerControllerVSI) that define referencing attributes as dependencies. The new attribute system currently has no way to express that a dynamic attribute depends on the attributes it's derived from, so this needs to be added.
    • implement setReference method
  • From the various SynchronGenerator classes, some of them have no header file at all while others derive from Base::SynchronGenerator in the header, but try to call the child constructor of some nonexistant class SynchronGeneratorBase. Currently, none of these classes should compile: DP::Ph3::SynchronGeneratorDQSmpl, DP::Ph3::SynchronGeneratorVBR, DP::Ph3::SynchronGeneratorVBRStandalone, EMT::Ph3::SynchronGeneratorDQSmpl, EMT::Ph3::SynchronGeneratorDQSmplCompSource, EMT::Ph3::SynchronGeneratorVBRSmpl, EMT::Ph3::SynchronGeneratorVBRStandalone
    • will be deleted by Jan
  • To be continued...

@JTS22
Copy link
Contributor

JTS22 commented Feb 4, 2022

Most points mentioned above are now marked in the code for PR #62 using the keywords DEPRECATED, FIXME or CHECK.

@m-mirz m-mirz moved this to In Progress in DPsim Feb 16, 2022
@m-mirz m-mirz added this to DPsim Feb 16, 2022
@m-mirz m-mirz removed this from DPsim Feb 16, 2022
m-mirz added a commit that referenced this issue Apr 25, 2022
This PR introduces a new attribute system to DPsim. Attributes are variable properties that can hold some numerical value, which can then be accessed from the outside by the simulation scheduler or by other components. Attributes can be computed from other attributes, therefore forming a dependency graph which is used in the scheduler to determine the execution order of tasks.

With this PR, every attribute can either be static or dynamic. Static attributes do not have any immediate dependencies, while dynamic attributes can be entirely derived from other attributes. For more details on the distinction between dynamic and static attributes, see this comment in issue #61

The usage of attributes in the component code remains roughly the same, however there are some differences:

- Attributes are public member variables of the components they belong to and they usually have type const Attribute<T>::Ptr.
- Attributes are created in the constructors initialization list. A new attribute can be created with the Attribute<T>::create and Attribute<T>::createDynamic methods.
- To acquire a (mutable) reference to an attribute's underlying data, the dereference operator has to be used twice on the attribute member variable, resulting in **mAttribute.
- Derived attributes can be created using the various derive-functions on the attribute object.
@m-mirz
Copy link
Contributor Author

m-mirz commented Jun 29, 2022

Closed by #62. Follow up on code quality issues in new issue #97.

@m-mirz m-mirz closed this as completed Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants