fiat
fiat copied to clipboard
FEniCS
Order-of-magnitude slowdown in FIAT
Original report by Andrew McRae (Bitbucket: amcrae, GitHub: amcrae).
Hello all,
We at Firedrake recently incorporated the changes between 5500635 and ec68699 into our fork of FIAT (i.e. all of Nico/Aslak's recent changes)
After running our test-suite through cProfile, the time spent in FIAT's polynomial_set init or 'below' jumped from 1.28% to 18.21%, accompanied by a 15-20% increase in the total test time. Nearly all the time is spent in calls to numpy_lambdify and calls to form_derivative, both in expansions.py.
Any idea what's happened?
Original comment by Jan Blechta (Bitbucket: blechta, GitHub: blechta).
FIAT has virtually no tests.
I've already started unit testing framowork (continuing the work of Aslak and Nico). It's in master. After a long time we are also running regression test, see #3. So this has been improved recently.
Expand testing to reach decent level of coverage.
I wasn't tough enough in email discussion with Firedrake people to require them to write tests for their additions. I'll try to cover FFC elements being moved to FIAT in future.
Original comment by Jan Blechta (Bitbucket: blechta, GitHub: blechta).
FIAT now has a unit test framework, thanks to Aslak, Nico, David, Miklós, Garth, etc.
Second task is now to write unit tests. Let's not accept new features without tests.
Original comment by Garth N. Wells (Bitbucket: garth-wells, GitHub: garth-wells).
@blechta I think we could do with a new 'contributing' guide to help contributors with the process, e.g. adding tests. Something could be added to https://bitbucket.org/fenics-project/docs.
Original comment by Jan Blechta (Bitbucket: blechta, GitHub: blechta).
I'm not sure that it needs a guide. Do you think that anybody, who is able to understand a relevant piece of code and contribute there, will not be able to go through the test dir and add something relevant there when asked to do that by an integrator? I think that most important is discipline of developers/integrators not to merge anything without appropriate coverage.
Original comment by Lawrence Mitchell (Bitbucket: wence, GitHub: wence).
Yes, but it's easier for everyone if the requirement to add tests is spelt out for new contributors. And also it reminds new developers/integrators that they should pay attention to features arriving without tests.
Original comment by Jan Blechta (Bitbucket: blechta, GitHub: blechta).
Yes, that's already stated here https://fenicsproject.org/contributing/#testing. If we would like to move that to RTFD page I'd suggest to file a more general issue stating what should be done in these regards.
Original comment by Garth N. Wells (Bitbucket: garth-wells, GitHub: garth-wells).
I think it could be stated much more clearly and with more detail on the work flow, making pull requests, test framework, etc.