-
Notifications
You must be signed in to change notification settings - Fork 565
Implement vector fitting to replace external vectfit
package
#3493
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
base: develop
Are you sure you want to change the base?
Conversation
This PR is currently a draft. The code is a copy-pasting of the suggestion by chatGPT provided by Paul. |
Hello guys, I've taken the time to re-implement the code using NumPy and SciPy. The unit tests are adapted from Jingang Liang’s implementation for consistency. To validate the results, I generated WMP data from ENDF/B-VII for both Na-23 and Fe-56, and ran both implementations on these datasets. I will soon share plots comparing the absolute error in absorption cross sections between the two codes but there almost 0. I also benchmarked the compute time, though I haven’t yet post-processed those results. In any case, I believe this additional performance data isn’t critical for this PR. |
vectfit
package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@azimgivron Thanks so much for going through this conversion! I made a few small updates on your branch but otherwise it looks pretty good. I do have one important question about a scaling below (wondering if this is affecting the results) below:
def test_large(large_test_data): | ||
"""Test vectfit with a large set of poles and samples""" | ||
s, f, weight, init_poles = large_test_data | ||
poles_fit, residues_fit, _, f_fit, _ = vectfit(f, s, init_poles, weight) | ||
assert np.allclose(f, f_fit, rtol=1e-3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test runs a bit long for my liking. Is there any way we can do the same kind of stress testing but with a shorter test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
large_test_data
is a fixture parameterized by Ns
and N
. You may reduce their values as needed; however, it will no longer serve as a stress test. I’m not sure whether it still makes sense to retain it.
Description
This pull request removes the dependency on the unmaintained external
vectfit
package (https://github.com/liangjg/vectfit) used in OpenMC's windowed multipole (WMP) infrastructure. In its place, a vector fitting is introduced based on thevectorFitting.py
module fromscikit-rf
project.The implementation will be made in several steps as suggested by Paul:
This change improves long-term maintainability and compatibility of OpenMC.
Fixes #3487
Checklist