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

Adding region and institution hierarchy to graphs/subgraphs #197

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

stompsjo
Copy link
Contributor

Hello all! I noticed that the default cymetric.flow_graph was not giving me quite the flexibility I wanted and thought I would offer this PR in case anyone else was interested. Here's a summary of what these changes do:

  1. All unique agents in a Cyclus output will be plotted in the flow graph. That means that if you have five PWRs, there will be five PWR nodes in your graphviz output. This is on account of looping over the IDs produced by evaler.eval("AgentEntry") rather than with evaler.eval("AgentEntry")["Prototype"].tolist(). Maybe we could include another input to flow_graph that disables this functionality.
  2. Regions and institutions will be plotted as subgraphs, rather than as disconnected nodes. This requires using the dot.subgraph(name="cluster_") syntax, but I think it results in a more organized graph (see example below). Again, all unique regions and institutes in your simulation will be plotted here. I took the liberty of setting some default visualization styles (dotted squares for regions, gray-filled squares for institutions) but I could imagine us supporting flexibility for this feature in the future.
  3. Using strict=True in the graph ensures that only unique edges are plotted, i.e. if multiple transactions take place between the same two nodes, only one of those edges is plotted. I don't recall if this was a concern before, but I think it might keep things safe from producing vast spaghetti plots.

The default engine/solver of graphviz does a good job of organizing the rendered plot, but I have noticed more edge overlap when using this subgraph structure. I have attached an example plot using the tutorial exercise from fuelcycle.org. You can recreate this image by running cyclus example.xml and then the following code:

import cymetric as cym

db = cym.dbopen('cyclus.sqlite')
evaler = cym.Evaluator(db, write=False)
dot = cym.flow_graph(evaler)
dot.render('example', format='png', cleanup=False)

example

Copy link

github-actions bot commented Jun 25, 2024

Build Status Report

Build FROM cycamore_20.04_apt/cycamore:latest - Success
Build FROM cycamore_20.04_apt/cycamore:stable - Success
Build FROM cycamore_20.04_conda/cycamore:latest - Success
Build FROM cycamore_20.04_conda/cycamore:stable - Success
Build FROM cycamore_22.04_apt/cycamore:latest - Success
Build FROM cycamore_22.04_apt/cycamore:stable - Success
Build FROM cycamore_22.04_conda/cycamore:latest - Success
Build FROM cycamore_22.04_conda/cycamore:stable - Success

@gonuke
Copy link
Member

gonuke commented Jun 27, 2024

Thanks @stompsjo for this contributions. Can you please add an entry to the Changelog? I also wonder why this fails on conda? That may be a question for @bennibbelink

@bennibbelink
Copy link
Contributor

bennibbelink commented Jun 28, 2024

The errors occuring are: ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

Initially I thought it might be related to the recent switch to mambaforge, however the error is occurring on the stable version as well (which still uses miniconda). This stackoverflow post suggests we may need to pin the numpy version for now... I'm guessing just for as long as it takes pandas to catch up and release a version compatible with numpy==2.0.0

EDIT: Pandas 2.2.2 should be compatible with numpy 2.0. Looking into this more

@bennibbelink
Copy link
Contributor

To the best of my knowledge this is the dependency situation:

  • Cymetric is installed with the latest versions of pandas (2.2.2) and numpy (2.0.0) which are compatible with each other
  • Cyclus is installed with the latest version of pandas but not the latest version of numpy (1.26.4) because pytables does not yet support numpy==2.0.0

An issue was made last week in the PyTables repo regarding this.

Until that issue is resolved... if we want CI to pass I think the easiest fix is to pin numpy<2.0.0 in Cymetric's pyproject.toml. @gonuke does this seem like a reasonable temporary fix?

@gonuke
Copy link
Member

gonuke commented Jul 2, 2024

That's fine as long as we have a way to remember to check on this as the compatibility progresses

@bennibbelink
Copy link
Contributor

Sounds good. I will make a PR with the temporary fix, as well as an open issue to check on pytables compatibility

This was referenced Jul 2, 2024
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One little question...

cymetric/graphs.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@stompsjo stompsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gonuke I pushed a couple other updates inspired by your suggestion.

Comment on lines +67 to +69
institutions = agents_[
(agents_["ParentId"] == region_id) & (agents_["Kind"] == "Inst")
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with using Kind to filter for Region, we can also use Instto ensure that we are filtering the appropriate agents.

Comment on lines +89 to +92
facilities = agents_[
(agents_["ParentId"] == institution_id)
& (agents_["Kind"] == "Facility")
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise for facilities.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks @stompsjo

@gonuke gonuke merged commit c81155c into cyclus:main Jul 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants