Skip to content

Convergence tests between Rust and C++ implementations - #6

Merged
Alex-J-Brown merged 13 commits into
trm-astro:mainfrom
AstroTeutloff:main
Aug 26, 2026
Merged

Convergence tests between Rust and C++ implementations#6
Alex-J-Brown merged 13 commits into
trm-astro:mainfrom
AstroTeutloff:main

Conversation

@AstroTeutloff

Copy link
Copy Markdown
Contributor

I've added a basic implementation of test_convergence_cpp.py, which tests convergence by loading a .mod file and a c++ solution and taking the difference between the c++ solution and the calculated Rust light curve. Difference is taken with pytest.approx to avoid floating point imprecision.

Included modfiles and solutions for four different systems.
Fixed minor formatting error in pytest convergence section
@AstroTeutloff

Copy link
Copy Markdown
Contributor Author

Important

Planning to add pytest.mark.parametrize for parametrization of the tests.

@AstroTeutloff

AstroTeutloff commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

TODO:

Add readme file that gives credit and how to integrate

@AstroTeutloff

Copy link
Copy Markdown
Contributor Author

TODO:

Set relative difference for pytest.approx to $10^{-8}$

@Fabmat1

Fabmat1 commented Aug 18, 2026

Copy link
Copy Markdown

PTF1 J0823+0819, sdB + WD, P = 87.5 min (TESS, 786.5 nm) (ellipsoidal no eclipses)
PG 0941+280, sdB + WD, P = 0.3148 d (TESS, 786.5 nm) (ellipsoidal+eclipses by WD)
FBS 2303+344, sdB + dM, P = 0.1988 d (TESS, 786.5 nm) (reflection effect)
FBS 2303+344, same system in ZTF zr (648 nm)

@StuartLittlefair

Copy link
Copy Markdown
Contributor

This is looking good! You'll want to remove the pycache folder from the list of commits.

We'll want to create a doc with information about the objects used in the end-to-end tests vs cpp-lcurve at some point, but that will have to wait for the framework of some documentation, so maybe it's for another pull request

…p_convergence.py accordingly/ Added small README.md for contributing

I've changed the file structure such that modfiles and solutions of a single object are always lumped together, and separated into dictionaries named after the object. This allows us to use pytest.mark.parametrize to automate the tests a little better.

I've also added a short destription of the directory and how to contribute to the testing suite.

Removed __pycache__ directory because it was wrongly included in tracking due to blanket `git add`.
@AstroTeutloff

Copy link
Copy Markdown
Contributor Author

ah, yes ofc. The old blanket git add tests lol. Removed it now and did some commenting and renovation of the directories. Should all in all be simpler to use and contribute now. Tests still pass nicely. I've changed to testing abs since I'm not super sure how rel is expected to work, I couldn't get it to trip up. Happy to change that back once we have a better understanding of it :)

@Alex-J-Brown

Copy link
Copy Markdown
Member

I've had a bit of a play around with this and think I understand a way to do it. I've attached a plot of the Rust model minus the C++ all divided by the C++ model. This has a maximum deviation of ~4.72e-10.

rust_cpp_comparison

np.zeros_like(lc.total) == approx((lc.total-ym)/ym, abs=4.8e-10) returns True and np.zeros_like(lc.total) == approx((lc.total-ym)/ym, abs=4.7e-10) returns False so that all seems to make sense. The question then is to decide what tolerance to aim for. 1e-9 is probably the right call but going lower would obviously be more sensitive to any issues that occur (but also maybe false positives). For the few models that I've looked at, the maximum deviation seems to be about 5e-10 but we may get some models with no problems that give larger deviations than that.

@Alex-J-Brown

Copy link
Copy Markdown
Member

Alternatively we can do lc.total == approx(ym, rel=4.8e-10, abs=1e-100) i.e. set the absolute tolerance so small that it never passes on absolute tolerance and therefore only the relative tolerance matters. This also then matches the expected cutoff based on the above model and is probably a more readable way of doing it

@AstroTeutloff

Copy link
Copy Markdown
Contributor Author

Alternatively we can do lc.total == approx(ym, rel=4.8e-10, abs=1e-100) i.e. set the absolute tolerance so small that it never passes on absolute tolerance and therefore only the relative tolerance matters. This also then matches the expected cutoff based on the above model and is probably a more readable way of doing it

This only triggers the test to fail for me when I set rel=4.5e-11, well below the threshold at which it should fail. I can commit that change, if all are for that.

Also, the unit tests fail because we don't have numpy as a dependency. I'd rather not include it in the dependencies if it is not used anywhere else, this stackoverflow thread shows how to handle this somewhat nicely. I can add this to the pyproject.toml as well, if wanted, so we hopefully don't break the CI.

@Alex-J-Brown

Alex-J-Brown commented Aug 24, 2026

Copy link
Copy Markdown
Member

could you plot the two models against each other and also make a plot of (lc.total-cpp) / cpp? I wonder if there's something we're missing about what we're actually comparing in these tests

@Alex-J-Brown

Copy link
Copy Markdown
Member

@Alex-J-Brown

Copy link
Copy Markdown
Member

@AstroTeutloff

Copy link
Copy Markdown
Contributor Author

Changed inputs to the legacy formats now, tests relative accuracy to $9\times10^{-9}$

@Alex-J-Brown

Alex-J-Brown commented Aug 25, 2026

Copy link
Copy Markdown
Member

Ok nice, could you add the python dependencies needed for the tests to the tests dependency group in the pyproject.toml and then I can merge the PR?

@Alex-J-Brown
Alex-J-Brown merged commit 896526e into trm-astro:main Aug 26, 2026
7 checks passed
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.

4 participants