Make systems/inputs/outputs/connections editable

[lukasmu]

Purpose

The goal of this PR is to make systems, inputs, outputs, and connections editable.
This makes it possible to generate an initial XDSM and afterwards similar XDSMs with some changes.

Example

from pyxdsm.XDSM import XDSM, FUNC, LEFT

x = XDSM()

f = x.add_system("F", FUNC, "F")
g = x.add_system("G", FUNC, "G")
c = x.connect("F", "G", "x, y")
f_out = x.add_output("F", "f^*", side=LEFT)
g_out = x.add_output("G", "g^*", side=LEFT)
x.write("initial")

f.label = 'F_{new}'
f_out.label = 'f^*, z'
g.faded = True
g_out.faded = True
c.faded = True
x.write("new")

Initial:

New:

Expected time until merged

Not urgent. Whenever you like.

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

See example above.

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[ewu63]

I'm going to defer to @marcomangano for review, but my initial thought is that this is a great feature to have. Two quick comments:

• Not 100% sold on adding a dependency, even though it is available on pyPI, but I think this is okay if there are no viable alternatives
• Perhaps the API can be improved a bit, by accessing attributes of the XDSM object rather than keeping track of additional objects.

[lukasmu]

Thanks for the input!

• The recordclass dependency offers a convenient drop-in replacement for the currenlty used namedtuple. Here's a version using the built-in dataclass (https://docs.python.org/3/library/dataclasses.html) instead of recordclass/ namedtuple: https://github.com/lukasmu/pyXDSM/tree/nodep. However, the dataclass version might not be 100% backwards compatible with namedtuple version (while the recordclass version is 100% backwards compatible).
• I also thought of making the systems attribute of the XDSM object an ordered dict (https://docs.python.org/3/library/collections.html#collections.OrderedDict) where the keys would be the node_names of the systems. This way one could easily access the systems. This would be a breaking change though. Alternatively, it is possible to add a getter method for finding system etc. (but this does not feel really "pythonic")

[marcomangano]

Thanks for the PR @lukasmu, and sorry for my late reply! I agree this is a very nice feature to have.
My comments so far:

• Is the backwards compatibility issue with dataclasses related to the attributes being not indexed anymore, so we need something like you did here in _build_process_chain? I would not mind that change as it makes the code more explicit and understandable tbh. I don't think many users access such information from their scripts right now anyway and, if it's the case, it is an easy fix. However, I understand your concerns and probably recordclass remains the best option if we don't introduce other breaking changes.
• I agree with @nwu63 that not having to track all the objects of interest would make the user code cleaner. We could add an update method that finds the correct object and updates the attributes of interest. Your example above could become something like:

from pyxdsm.XDSM import XDSM, FUNC, LEFT

x = XDSM()

x.add_system("F", FUNC, "F")
x.add_system("G", FUNC, "G")
x.connect("F", "G", "x, y")
x.add_output("F", "f^*", side=LEFT)
x.add_output("G", "g^*", side=LEFT)
x.write("initial")

x.update("F", label='F_{new}')
x.update("left_output_F", label=''f^*, z'')
x.update("G",faded=True)
x.update("left_output_G", faded=True)
x.write("new")

Not sure about how to do this elegantly because, as you hinted at, self.systems and self.connections are lists while the others are dict, and changing the first ones to orderedDict might be a major refactoring that goes beyond the scope of this PR

Anyway, I do not have strong opinions on any of the above and I think we should merge this feature sooner than later!
The only actual request I have is that, whatever we converge on, we add a short example of the new feature to the docs.

[lukasmu]

@marcomangano thanks for the feedback!

If you don't mind making changes that break backward compatibility, I would indeed propose to make the following to changes:

• Replacing namedtuple by dataclass (as done in https://github.com/lukasmu/pyXDSM/tree/nodep)
• Transforming the systems attribute of the XDSM object from a list to an OrderedDict

I think the refactoring effort (also for the latter) is limited. But the interface would be nicely explicit and would not depend on additional packages afterwards (maybe we can even drop the numpy dependency in the same go). A changelog file should be added as well and a new major version should be released afterwards.

I will try to work on this during the holidays and update this PR accordingly.

[lamkina]

A few comments I want to add to be explicitly clear about what's happening here.

1. Named tuples are immutable, so storing attributes with named tuples is not a great idea for flexibility. You can however, get around this by doing something like this:

for i, sys in enumerate(x.systems):
    if sys.node_name in ["core", "perf", "bc"]:
        x.systems[i] = sys._replace(faded=True)

The above snippet allows you to alter the attributes of the named tuple (because nothing in python is truly immutable if you try hard enough).

A more straightforward approach would be to make python classes for each of the System, Input, Output, and Connection objects. You can manually add their attributes or use dataclass to do it for you.

To adress changing the system object to an OrderedDict, all standary python dictionaries are ordered after python 3.7. If we are smart about doing this, I don't think it needs to be backward breaking. Might take some extra effort under the hood to make sure existing user-facing functionality is the same.

[lamkina]

Thanks for the PR @lukasmu, and sorry for my late reply! I agree this is a very nice feature to have. My comments so far:

• Is the backwards compatibility issue with dataclasses related to the attributes being not indexed anymore, so we need something like you did here in _build_process_chain? I would not mind that change as it makes the code more explicit and understandable tbh. I don't think many users access such information from their scripts right now anyway and, if it's the case, it is an easy fix. However, I understand your concerns and probably recordclass remains the best option if we don't introduce other breaking changes.
• I agree with @nwu63 that not having to track all the objects of interest would make the user code cleaner. We could add an update method that finds the correct object and updates the attributes of interest. Your example above could become something like:

from pyxdsm.XDSM import XDSM, FUNC, LEFT

x = XDSM()

x.add_system("F", FUNC, "F")
x.add_system("G", FUNC, "G")
x.connect("F", "G", "x, y")
x.add_output("F", "f^*", side=LEFT)
x.add_output("G", "g^*", side=LEFT)
x.write("initial")

x.update("F", label='F_{new}')
x.update("left_output_F", label=''f^*, z'')
x.update("G",faded=True)
x.update("left_output_G", faded=True)
x.write("new")

Not sure about how to do this elegantly because, as you hinted at, self.systems and self.connections are lists while the others are dict, and changing the first ones to orderedDict might be a major refactoring that goes beyond the scope of this PR

Anyway, I do not have strong opinions on any of the above and I think we should merge this feature sooner than later! The only actual request I have is that, whatever we converge on, we add a short example of the new feature to the docs.

The idea of adding an x.update() method seems very reasonable. Then we can change the way we track systems/connections under the hood and the user facing interface should remain the same.

[lukasmu]

A more straightforward approach would be to make python classes for each of the System, Input, Output, and Connection objects. You can manually add their attributes or use dataclass to do it for you.

That's exactly what I want to do/did in https://github.com/lukasmu/pyXDSM/tree/nodep.

To adress changing the system object to an OrderedDict, all standary python dictionaries are ordered after python 3.7. If we are smart about doing this, I don't think it needs to be backward breaking. Might take some extra effort under the hood to make sure existing user-facing functionality is the same.

The idea of adding an x.update() method seems very reasonable. Then we can change the way we track systems/connections under the hood and the user facing interface should remain the same.

I would still choose OrderedDict because order is guranteed and it has methods specialized for rearranging order. We might have different opinions about what is considered backwards compatible. In my opinion changing the systems list to a dict/OrderdDict is already breaking backwards compatibility since the systems attribute (while not directly user-facing) is not really a private attribute (designated by a leading underscore).