Fusion-Power-Plant-Framework / bluemira

Bluemira is an integrated inter-disciplinary design tool for future fusion reactors. It incorporates several modules, some of which rely on other codes, to carry out a range of typical conceptual fusion reactor design activities.
https://bluemira.readthedocs.io/
GNU Lesser General Public License v2.1
68 stars 16 forks source link

Consider module dependency structure #767

Open hsaunders1904 opened 2 years ago

hsaunders1904 commented 2 years ago

Description of issue / requirement to address

We should consider the module layout of bluemira, and decide which modules should be dependent on which others. We should try to minimise the number of looped dependencies to avoid import loops.

A graph of the module dependency structure can be generated using the snakefood3 package and GraphViz.

To generate the graph:

For example:

>> pip install snakefood3 graphviz

>> echo -e "bluemira.balance_of_plant\nbluemira.base\nbluemira.builders\nbluemira.codes\nbluemira.display\nbluemira.equilibria\nbluemira.fuel_cycle\nbluemira.geometry\nbluemira.magnetostatics\nbluemira.materials\nbluemira.mesh\nbluemira.radiation_transport\nbluemira.utilities" > module-groups
>> snakefood3 bluemira/ bluemira --group module-groups > graph.dot
>> dot graph.dot -T png -o graph.png

produces: deps

Output:

je-cook commented 2 years ago

That looks like a great stating point! I think its possible to organise dot graphs more in a layer type fashion. Looks like we need to do some organisation though....

ivanmaione commented 2 years ago

Not sure what is the best way to address this issue; however, I was looking at the graph posted by @hsaunders1904 and I was wondering why codes depends on geometry (it should not in my opinion). Looking at the code, I found that geometry is used in two points:

def setup_radial_build(run, width=1.0):
    """
    Plots radial and vertical build of a PROCESS run
    Input: Dictionary of PROCESS output
    Output: Plots
    """
    from bluemira.geometry._deprecated_loop import Loop
    ...

def fix_wire(wire, precision=EPS, min_length=MINIMUM_LENGTH): ...


This is not so critical and we could also leave things are they are, but I think this could be a simple test case to remove a "not necessary" dependence. Any thoughts?
CoronelBuendia commented 2 years ago

Hi @hsaunders1904 this is a great start, thanks for taking the initiative. Definitely shows we need to get a better handle on this. I think the plot could maybe be made neater, and similar to @ivanmaione I think that this is already highlighting some strange inter-dependencies (@ivanmaione 's example above and equilibria depends on builders somehow?). We can address these one by one.

I think this would be worth adding to the documentation (maybe in the developer documentation) if we can make the graph cleaner.

je-cook commented 2 years ago

It may be good to include some future iteration of the above figure in our docs once we've cleaned up (especially if it can be autogenerated). Possibly it would be nice to have clickable nodes that expand for each top level module. See https://andrewmellor.co.uk/blog/articles/2014/12/14/d3-networks/. I think it would add a lot to a first docs glance for any would be developer. The above may be overkill however :slightly_smiling_face:

CoronelBuendia commented 2 years ago

Can confirm that the equilibria module dependency on builder module is because of some planned deprecation and will disappear at some point

je-cook commented 2 years ago

get module names

for ii in `echo */` 
    do
        echo bluemira.${ii:0:(-1)}
    done > module_groups
je-cook commented 2 years ago

Getting there! Still some circular things with:

base <-> display <-> utilities <-> codes <-> geometry <-> mesh

graph

je-cook commented 2 years ago

The approach in #1628 could be used more widely to deal with this