fix: Proposed review changes to #485

[acl-cqc]

No description provided.

[acl-cqc]

The awkward thing is that this makes run_int_fn (the same as before but) different from run_float_fn, so perhaps they should be named differently. check_int_fn perhaps, although it'd be less change to rename the new run_float_fn instead....thoughts?

[doug-q]

Completely agree. maybe eval_float_fn? It's not too bad to rename run_int_fn -> check_int_fn. Alternatively, we could perhaps move the pytest.approx (nice btw!), would run_float_fn(package, expected = pytest.approx(x)) work?

[acl-cqc]

This is a straightforward reversion of an unnecessary change. (The code is not executed, the result does not have to be true, and division is valid.)

[doug-q]

Yes indeed, this is leftover from a moment where division was banned.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.67%. Comparing base (a47ba8a) to head (e5b0601).

Additional details and impacted files

@@                 Coverage Diff                 @@
##           ss/fix-angle-lower     #506   +/-   ##
===================================================
  Coverage               91.67%   91.67%           
===================================================
  Files                      59       59           
  Lines                    6138     6138           
===================================================
  Hits                     5627     5627           
  Misses                    511      511           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[doug-q]

Thanks Alan, I really appreciate the help here.

[doug-q]

Yes indeed, this is leftover from a moment where division was banned.

[doug-q]

Completely agree. maybe eval_float_fn? It's not too bad to rename run_int_fn -> check_int_fn. Alternatively, we could perhaps move the pytest.approx (nice btw!), would run_float_fn(package, expected = pytest.approx(x)) work?

[acl-cqc]

Completely agree. maybe eval_float_fn? It's not too bad to rename run_int_fn -> check_int_fn. Alternatively, we could perhaps move the pytest.approx (nice btw!), would run_float_fn(package, expected = pytest.approx(x)) work?

Hah! Yes, it's much easier than what I did. I was remembering an assert_close(a, b) but that's numpy not pytest. With pytest.approx, as you say, we don't need to change run_float_fn at all - just pass in a pytest.approx.

Alternatively we could modify run_float_fn to apply the pytest.approx itself (rather than at each callsite) - that would be good practice, I think (so we could have a run_float_fn fixture that takes optional rel and abs precision arguments and passes them onto pytest.approx), but I've not done that here. Thoughts?