Cloning Components in PowerElectronics Module

[abdourahmanbarry]

Description

This pull request implements clone functionality for components in the PowerElectronics module. This gives us the ability to create independent copies of components such that modifications to one do not affect the other.

Proposed changes

Each component implements the isCloneable and clone methods. The isCloneable method returns true if the component is cloneable and false otherwise. The clone method returns a pointer to a new object with the same state as the component from which it was cloned. After cloning a component, the user should set the data pointers for the clone.

Checklist

• All tests pass.
• Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
• The new code follows GridKit™ style guidelines.
• There are unit tests for the new code.
• The new code is documented.
• The feature branch is rebased with respect to the target branch.
• The CHANGELOG.md has been updated to reflect the changes. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further Comments

This functionality is useful for the partitioning implementation.

[superwhiskers]

this change looks pretty inoffensive. one comment, however:

why are we adding isCloneable for only one component (the Bus) to not support cloning? additionally, might it be better to implement this by using a subclass of CircuitComponent that implies cloneability, or something else? it's not like cloning is going to change at runtime, so it might be cleaner to add some structural way to ensure a component being used supports cloning without doing a runtime check by calling isCloneable.

[nkoukpaizan]

A few comments. At a higher level:

• I don't see anything preventing calling the copy constructor on non-clonable components.
• It's not clear to me what is the intended functionality of the clones. More documentation would be helpful.

[nkoukpaizan]

This should be in the Testing namespace. Perhaps a private method of CircuitComponentCloneTests?

[abdourahmanbarry]

Updated.

[nkoukpaizan]

Please add some documentation for this constructor.

[abdourahmanbarry]

I have added more documentation to this.

[nkoukpaizan]

We are currently not checking that the components are clonable before cloning...

[abdourahmanbarry]

I expect a user to call isCloneable before calling clone.

[abdourahmanbarry]

I have gotten rid of isCloneable. Now non-cloneable classes just throw an error as suggested by @alexander-novo .

[abdourahmanbarry]

this change looks pretty inoffensive. one comment, however:

why are we adding isCloneable for only one component (the Bus) to not support cloning? additionally, might it be better to implement this by using a subclass of CircuitComponent that implies cloneability, or something else? it's not like cloning is going to change at runtime, so it might be cleaner to add some structural way to ensure a component being used supports cloning without doing a runtime check by calling isCloneable.

I added clone to CircuitComponent alone for now since it is the one for which I have immediate use for.

I think having a cloneable subclass such as CloneableCircuitComponent makes sense. I can try doing that. This will also address some of @nkoukpaizan's comments as well.

[alexander-novo]

I talked to @abdourahmanbarry in person. I think it makes a lot more sense to have all components be cloneable and remove the isCloneable function. Then for components where the implementation of clone() is out of scope for this PR we can simply throw an error stating the function isn't implemented.

The problem with a CloneableCircuitComponent interface is that to call clone() you'd need to know you have a CloneableCircuitComponent. For cloning an entire system, you'd need to know all of the components are a CloneableCircuitComponent which would make component_type = CloneableCircuitComponent which is the same thing as just having all circuit components as CloneableCircuitComponent (modding out dynamic typecasting).

[abdourahmanbarry]

A few comments. At a higher level:

• I don't see anything preventing calling the copy constructor on non-clonable components.
• It's not clear to me what is the intended functionality of the clones. More documentation would be helpful.

I am using clone as part of my implementation for partition interfaces in the next pull request on this stack. I have also updated the implementation of the copy constructor such that it should be okay to call it independently of the clone. Let me know if these changes look good.