search

tBuLi / symfit

Symbolic Fitting; fitting as it should be.
http://symfit.readthedocs.org
MIT License
232 stars 17 forks source link

test_chained_min: relax test constraints #366

Closed stephanlachnit closed 1 year ago

stephanlachnit commented 1 year ago

This test fails on ppc64el otherwise, see e.g. https://ci.debian.net/data/autopkgtest/testing/ppc64el/s/symfit/32008561/log.gz (search for test_chained_min).

pckroon commented 1 year ago

Thanks. I have no issues with this at all. @tBuLi any concerns?

I would really appreciate your input on how to deal with these type numerical testing issues though, at a more general level. Does it make sense to just relax them until it works, or does that simply invalidate the tests?

Jhsmit commented 1 year ago

We could use something like skipif to run numerical tests with strict constraints only on one platform and python version?

stephanlachnit commented 1 year ago

I would really appreciate your input on how to deal with these type numerical testing issues though, at a more general level. Does it make sense to just relax them until it works, or does that simply invalidate the tests?

I guess it depends a bit and what you want to test/achieve with it. In this case I think it sounds reasonable, given that this is pretty much within what you can except with floating point precision.

When we talk about https://github.com/tBuLi/symfit/issues/348, this is argumentation however is not really valid, the test just refuses to work on i386 at all (and I'm still not entirely sure why).

I guess it boils down to: do you except the tests to show that it works in general? If so, then relaxing the requirements seems reasonable if the result is not too far off like in this case.

If you want to test the performance of the fitters, then relaxing requirements until it works does indeed invalidate the purpose of the tests. I don't know a good solution for this tbh, since I think a "failure" in that case is not critical as in the software in broken. But if these tests are there to test the performance, then running them as CI check (that has to pass) does not make a lot of sense.

pckroon commented 1 year ago

I guess it boils down to: do you except the tests to show that it works in general?

Great question :) We (I at least) do not want to test the scipy optimizers. I do want to test the symfit side of code. I guess the summary is that our current tests are rather integrative in nature, which is... not ideal. My vote is that this change is OK; and we need to revisit our tests (but who has the time...)

@tBuLi Please merge and release when able :)

tBuLi commented 1 year ago

I agree that we should only be testing our symfit code, not scipy. Unfortunately this is a bit hard to untangle, and there are a lot of tests that are indeed kind of written as performance tests even though their goal is just to make sure our code is executed correctly.

So I have no problem with this PR, improving the tests one step at a time ;). I don't think we need a new release just for this though, or do we?

stephanlachnit commented 1 year ago

So I have no problem with this PR, improving the tests one step at a time ;). I don't think we need a new release just for this though, or do we?

Nah that's fine, thx!