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

Replace deprecated scipy.integrate.quadrature in the TestLineShapes #457

Merged

Conversation

vsnever
Copy link
Member

@vsnever vsnever commented Aug 2, 2024

This PR replaces deprecated scipy.integrate.quadrature with Cherab's own GaussianQuadrature integration method in the TestLineShapes test case.

Copy link
Member

@jacklovell jacklovell left a comment

Choose a reason for hiding this comment

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

My worry here is that the lineshape test now depends on GaussianQuadrature functionality so is getting closer to an integration test than a unit test. OK, we do at least have test coverage of GaussianQuadrature but personally I would prefer to use a thoroughly tested scipy function rather than another Cherab utility to isolate the lineshape from the integration tests.

Having done a bit more research, it appears scipy.integrate.quad is genuinely a better solution than scipy.integrate.quadrature, not just a rename as I initially thought, so I'd suggest swapping to scipy.integrate.quad instead. It's been available for ages (since at least 0.13, possibly earlier) so there's no worries about backwards compatibility with old Python/Scipy versions.

@Mateasek
Copy link
Member

Mateasek commented Aug 2, 2024

I agree with with @jacklovell here.

The test cases for us pose as some kind of standards to be matched. This is why they should be analytical solutions where possible. Where not possible, the tests should be constructed with widely accepted external libraries. Then, the tests warn us if any new code causes discrepancies out of set tolerances. Performing tests with "in-house" functionality increases probability of missing a problem

For example, if there is a PR which introduces changes to the GaussianQuadrature which in turn causes discrepancies with line shape standards in this case, the proposed changes would cause the test to completely miss it and that is in my opinion wrong.

For these reasons, I would suggest using scipy functionality (as suggested by @jacklovell ) because it is tested by a wider user pool, can be assumed as a kind of standard here and is not "in-house".

@vsnever
Copy link
Member Author

vsnever commented Aug 2, 2024

I agree. When we use a scipy function, we are testing both the line shape model and the integration itself, and it's better. I replaced quadrature() with quad().

GaussianQuadrature is based on scipy's quadrature() algorithm, so they give identical results no matter what the relative tolerance is set to, as long as it is the same for both. The quad() algorithm is slightly different, so we should now compare results within the tolerance threshold only.

Copy link
Member

@Mateasek Mateasek 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 updating the tests to use the quad function. I have one minor suggestion which I didn't catch last time.

@@ -296,7 +296,7 @@ def test_stark_broadened_line(self):
line = Line(deuterium, 0, (6, 2)) # D-delta line
target_species = self.plasma.composition.get(line.element, line.charge)
wavelength = 656.104
integrator = GaussianQuadrature(relative_tolerance=1.e-5)
integrator = GaussianQuadrature(relative_tolerance=1.e-8)
Copy link
Member

Choose a reason for hiding this comment

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

1.e-8 is used in the quad calls too, please consider using a variable instead (e.g. relative_tolerance), if the values are supposed to be equal here and below. It could save some trouble in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @Mateasek. I added the relative_tolerance variable. Also, in this case it is better to check for relative error rather than absolute error, so I changed that too.

@jacklovell jacklovell merged commit 2fd8b49 into cherab:development Aug 7, 2024
8 checks passed
@vsnever vsnever deleted the test/remove_deprecated_scipy_quadrature branch August 30, 2024 16:24
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