phys201 / multstars

A package for parameter estimation of stellar multiplicity properties
GNU General Public License v3.0
0 stars 0 forks source link

Tests should test specific values, not ranges #19

Closed barkls closed 4 years ago

barkls commented 4 years ago

You have good test coverage of your package right now, but most of the tests check that the output is in a specific range and not the actual value. One of the goals of tests is to confirm that the code behaves the same way when you make changes, and right now you aren't sensitive enough to detect changes such as defining a Gaussian a different way, for example.

One reason you might be using ranges is because the tests aren't returning the exact same value each time due to sampling. If that's the case, you should be setting a random seed for your tests to make sure they are completely deterministic. That will also let you cut down on the number of pymc3 samples and speed up your tests.

1ynd0n commented 4 years ago

It's not clear to me whether one should be testing for whether parameter estimation works or whether the code runs; in the case of the latter, I am in agreement that setting the seed, etc. should work. But if you also want to test, e.g., a MAP value, how does one do that without looking over ranges?

barkls commented 4 years ago

Both are valid targets for testing. For the purposes of the course we are focusing on unit tests, which are low level tests that check basic functioning of the code. Higher level integration and system tests confirm that everything works properly together to give the right results, but we are hoping to cover those bases with the tutorial notebook and not necessarily on tests. Design of software testing and QA are an entire field on their own!

On Sun., Apr. 26, 2020, 15:12 1ynd0n, notifications@github.com wrote:

It's not clear to me whether one should be testing for whether parameter estimation works or whether the code runs; in the case of the latter, I am in agreement that setting the seed, etc. should work. But if you also want to test, e.g., a MAP value, how does one do that without looking over ranges?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/phys201/multstars/issues/19#issuecomment-619608266, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJ7CIVDU7DLE3BPWUYDROSBRNANCNFSM4MOK3C3A .

vditomasso commented 4 years ago

Model tests have been scaled back and again have poor coverage. I can't figure out how to test basic functionality without running the full model, and I can't figure out how to incorporate a random seed like you described. This is definitely a lesson in the value of test-driven development

vnmanoharan commented 4 years ago

With pymc3, you can just include a random_seed= keyword argument when you call pm.sample(). Also, before you generate your synthetic data set, you can use np.random.seed() to set the seed, so that you get the same data set for each test.

vditomasso commented 4 years ago

Thank you! I've implemented those changes