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

In volume calculation mode, only load atomic weight ratio from data files #2741

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

paulromano
Copy link
Contributor

@paulromano paulromano commented Oct 20, 2023

Description

This PR is an alternative complementary to #2725.

@bam241 This is what I was thinking -- for volume calculation mode (which is used by openmc-plotter), we still open the .h5 files but only load the atomic weight ratio since this is all that is needed for the sake of computing densities. This won't yet allow users to run the plotter without having cross sections specified, but it at least does speed up the loading time significantly since all the cross section data itself is not loaded.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@shimwell
Copy link
Member

I see this PR is just 4 lines of code so that is a nice speed improvement for such a small modification

I guess #2725 is still preferable from the user point of view as it avoids the needs for having cross sections specified and be even faster as well.

@paulromano
Copy link
Contributor Author

@shimwell Yes, from a user's point of view #2725 would be nice, but I think it will take a bit more work to get things working with the plotter since the plotter still assumes atomic masses and normalized densities are available. This PR is actually complementary in the sense that we could merge both (updated my original description to reflect that). This will simply make things faster when cross section data is available.

@shimwell
Copy link
Member

This looks good to me, I just want to get some representative speed ups for peoples interest

@paulromano
Copy link
Contributor Author

@shimwell The time spent "loading cross sections" for a volume calculation (or for loading openmc-plotter) goes from some value T to basically 0, so the speedup is T/0 = ∞ 😉

@shimwell
Copy link
Member

shimwell commented Nov 27, 2023

A geometry with Eurofer material look around 5s to load for the volume calculation. Now it takes a fraction of a second.

Nominating this PR for biggest speed improvement per line of code change award

@shimwell shimwell added the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Nov 27, 2023
@shimwell
Copy link
Member

Will merge tomorrow morning (UK time) unless there are objections

@shimwell shimwell merged commit b66c6d4 into openmc-dev:develop Nov 28, 2023
16 checks passed
@paulromano paulromano deleted the volume-calc-awr-only branch November 28, 2023 12:39
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merging Soon PR will be merged in < 24 hrs if no further comments are made.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants