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

allowing no cross_section in volume calculation #2725

Closed
wants to merge 34 commits into from

Conversation

bam241
Copy link
Contributor

@bam241 bam241 commented Oct 11, 2023

as discussed during last call, this follows #2690 PR.
The aim of this is to allow Volume calculation, with limited capabilities when no cross_section are provided.

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)

include/openmc/constants.h Outdated Show resolved Hide resolved
src/volume_calc.cpp Outdated Show resolved Hide resolved
@bam241 bam241 marked this pull request as ready for review October 24, 2023 07:02
@bam241
Copy link
Contributor Author

bam241 commented Oct 24, 2023

@paulromano this should be good to review.

@bam241
Copy link
Contributor Author

bam241 commented Oct 27, 2023

I realised I didn't implement the test for this.

Looking at what test existed for the Volume calculation, they seemed pretty short in test_volume.py (just verifying that infinity is handle properly).

I am happy to implement test if you fell my addition made them necessary.
If tests are required, maybe this isn't the right place. In any case let me know

@bam241
Copy link
Contributor Author

bam241 commented Oct 27, 2023

I also just saw that one of the job failed:

The job running on runner GitHub Actions 15 has exceeded the maximum execution time of 360 minutes.

What should I do about this? trigger CI again with an empty commit ?

@paulromano
Copy link
Contributor

@bam241 Yes, you'll need a test for this capability (I tried it locally and it did not work out of the box for me). The test can live in test_volume.py but will be a bit more involved and should have the following logic:

  • Create a simple model with a volume calc
  • Unset cross sections (del openmc.config['cross_sections'])
  • Confirm that a volume calculation runs successfully
  • Confirm ability to load results into the Python VolumeCalculation class

@paulromano paulromano removed this from the v0.14.0 milestone Oct 30, 2023
@bam241
Copy link
Contributor Author

bam241 commented Nov 2, 2023

@paulromano I added the test, and fixed PR so it actually works.

The test is based on the geometry of the pincell tutorial. It might be overly complicated, but having the s_alpha_beta for the water actually allowed me to spot an potential bug in the summary.h5 writing.
let me know if you wish for a simpler geometry.

@shimwell
Copy link
Member

Thanks for adding those tests Bam.

I was looking through the open PRs piling up and I think this is the oldest PR that is also complete.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on in providing some more feedback here. I'm hesitant about merging this PR in its current design for the following reasons:

  • There is a desire longer-term to store atomic mass / isotopic composition data on the C++ side of the code, which would give us some defaults to fall back on even when no cross sections are available, in which case we could always run a volume calculation and still calculate atom densities.
  • I don't see the extra code here as being that beneficial for a relatively uncommon situation -- in most cases a user should have cross sections configured in which case there is no issue.
  • In volume calculation mode, only load atomic weight ratio from data files #2741 gives a very easy solution to the problem of slow initialization of the plotter due to cross section loading.

@shimwell
Copy link
Member

Fair enough, I can review #2741 tomorrow.

@bam241
Copy link
Contributor Author

bam241 commented Nov 28, 2023

@paulromano @shimwell let me know what you wish to do with this.

I can close it, or re-work it an other way.

@shimwell
Copy link
Member

I have a feeling knowledge learned or perhaps code here will be useful for when we store atomic mass / isotopic composition data on the C++ side of the code.

@shimwell
Copy link
Member

shimwell commented Dec 1, 2023

I'm going through the open PRs today and I think this one can be closed as we have merged #2741

#2741 significantly speeds up the loading of nuclear data and while this one totally removes the need for nuclear data I think Paul is right and we should do this by adding the atomic mass / isotopic composition data on the C++

@bam241 would you be interested in some follow on sponsored work to add atomic mass / isotopic composition data on the C++ ?

@shimwell shimwell closed this Dec 1, 2023
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