Skip to content

Improve performance for large models with DomainViewDict#138

Merged
pshriwise merged 5 commits into
openmc-dev:developfrom
paulromano:global-domainview
Apr 2, 2024
Merged

Improve performance for large models with DomainViewDict#138
pshriwise merged 5 commits into
openmc-dev:developfrom
paulromano:global-domainview

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

If you run the plotter on a model with lots of cells (the ITER E-lite model, for example), it spends a lot of time copying data around unnecessarily. The crux of the issue is this line:

cv = self.currentView = copy.deepcopy(self.activeView)

which deepcopies the entire PlotView object. This object holds dictionaries called cells and materials that map cell and material IDs to DomainView objects (one for each cell and material that appears in the model). When I plot the ITER E-lite model, which has >300k cells, those deepcopies take about 12 seconds on my machine, which means that every time I update the plot view, there is a lag of 12 seconds even when the plot resolution is super low.

This PR makes a fairly easy fix to this problem, which is to store a global dictionary that holds the default DomainView view settings that are created at startup separately from the PlotView object itself. If you make a modification to the domain view (e.g., changing the color of a material), it does get stored as part the PlotView object and will get copied around, but the default domain views remain untouched and don't get copied.

@paulromano paulromano requested a review from pshriwise March 25, 2024 18:36
@paulromano

Copy link
Copy Markdown
Contributor Author

Here's what it looks like working with E-lite on my laptop:
plotter_domainviewdict

@paulromano

Copy link
Copy Markdown
Contributor Author

Just added a commit which shaves off about ~7 sec of initialization time for the E-lite model

@paulromano

Copy link
Copy Markdown
Contributor Author

...and just made another commit that shaved another 7 sec off initialization. Generating the default DomainView objects for each domain took about 20 sec before for the E-lite model and now it's down to 5 sec.

@pshriwise

Copy link
Copy Markdown
Collaborator

Good idea to de-couple those from the PlotView's -- no reason we need to bring them along in the copy. Any reason we shouldn't put those global dictionaries on the PlotModel object?

@paulromano

Copy link
Copy Markdown
Contributor Author

@pshriwise The global dictionaries are accessed from the DomainViewDict class, which is separate from the PlotModel class

@pshriwise

pshriwise commented Mar 28, 2024

Copy link
Copy Markdown
Collaborator

@pshriwise The global dictionaries are accessed from the DomainViewDict class, which is separate from the PlotModel class.

Understood, but any reason not to make the DomainViewDict class an attribute of the PlotModel class? It seems it would be more in line with the MVC approach this app is intending to take.

@paulromano

Copy link
Copy Markdown
Contributor Author

Ok, I was able to come up with a better (I think?) solution here. Instead of storing the defaults as a global dictionary, they are stored as an attribute on DomainViewDict. To avoid the original issue with all the defaults being deepcopied, I overrode the __deepcopy__ method to make it only copy a reference of the defaults dictionary. Let me know what you think now @pshriwise

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just one small question about a change to the default pixel size, but then I think this is ready! 🚀

Thanks for this update! I tried out the E-lite model out (both before and after these changes) and it makes a big difference -- bring on the detail!

Comment thread openmc_plotter/plotmodel.py Outdated
self.height = height
self.h_res = 1000
self.v_res = 1000
self.h_res = 100

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to change the default pixel values permanently? I think for most models this default is manageable.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Come to think of it, being able to set the initial number of pixels might be a nice option to have -- making an issue...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That was unintentional, got a little overzealous with git commit -a 😊 But yes! we absolutely should have an option to change the default resolution

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, right one. Issue #139 created so we can tackle that later. Thanks!!

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the performance upgrade @paulromano!

@pshriwise pshriwise merged commit 8aa5067 into openmc-dev:develop Apr 2, 2024
@paulromano paulromano deleted the global-domainview branch April 5, 2024 07:23
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.

2 participants