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

update math function unit test with catch2 #2955

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

lhchg
Copy link
Contributor

@lhchg lhchg commented Apr 12, 2024

Description

Some interfaces do not need to be exposed to the python API, except for unit testing, so move math unit tests to C++ unit tests, which implemented in python before

Fixes #2943

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) 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)

@lhchg lhchg requested a review from paulromano as a code owner April 12, 2024 07:59
@lhchg lhchg changed the title update math unit test with catch2 update math function unit test with catch2 Apr 12, 2024
@lhchg
Copy link
Contributor Author

lhchg commented Apr 12, 2024

Sorry, seems some error will be introduced. I'm checking

@lhchg lhchg force-pushed the update_math_test branch 2 times, most recently from 30ffd7e to 909cdf1 Compare April 16, 2024 01:22
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.

Thanks for tackling this @lhchg! I haven't reviewed all the changes yet but noticed the test failure and see that it's related to the issue below:

tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

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

Awesome!

One benefit of this change is that calc_pn_c function at a among a few others requires temporary space to be allocated while particles are in flight. That's not really a great code design, since we could be using the recursive formula instead for scoring purposes. It's nice that this PR removes dependence on that behavior for testing. It would be more straightforward to just test the values (regardless of how they're obtained) rather than having the intermediate API that assumes a certain output format.

One thing worth addressing: is it possible any users are relying on these math functions in the C API?

tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
tests/cpp_unit_tests/test_math.cpp Outdated Show resolved Hide resolved
@lhchg lhchg force-pushed the update_math_test branch from 909cdf1 to 8dfb014 Compare April 22, 2024 05:53
@lhchg
Copy link
Contributor Author

lhchg commented Apr 22, 2024

Hi @paulromano @gridley

I updated the code to use hardcode for the reference part.
And I think if included a python script to generate the reference part, maybe this would introduce new python package dependencies when running the catch2 unit tests?

What do you think of this hardcode approach?

If you think it would be better to include a python script, I will continue to modify it.

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.

@lhchg Thanks for the updates on this! It looks good to me now.

@gridley Do you have any further requests on this one?

Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

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

Awesome changes! Nice to have the code simplified some like this.

@lhchg
Copy link
Contributor Author

lhchg commented Apr 28, 2024

There seems to be some dependency on the python math api

self = <openmc.polynomial.ZernikeRadial object at 0x7fe6bfe0dc90>, r = 0.196

    def __call__(self, r):
        import openmc.lib as lib
        if isinstance(r, Iterable):
            return [np.sum(self._norm_coef * lib.calc_zn_rad(self.order, r_i / self.radius))
                    for r_i in r]
        else:
>           return np.sum(self._norm_coef * lib.calc_zn_rad(self.order, r / self.radius))
E           AttributeError: module 'openmc.lib' has no attribute 'calc_zn_rad'

openmc/polynomial.py:82: AttributeError

I have fixed it

@lhchg lhchg force-pushed the update_math_test branch from 8dfb014 to 83422a8 Compare April 28, 2024 06:04
@paulromano paulromano enabled auto-merge (squash) June 19, 2024 13:44
@paulromano paulromano merged commit 3bedd04 into openmc-dev:develop Jun 19, 2024
15 checks passed
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace math function unit tests in Python with C++ tests
3 participants