search

AstroHuntsman / gunagala

4 stars 9 forks source link

Fix axis 18 WIP #20

Closed bazkiaei closed 5 years ago

bazkiaei commented 5 years ago

Learning how to work with pull request

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.2%) to 87.199% when pulling ee0e2650483ddea7be0b7c86fd18cc975a4f4b48 on bazkiaei:fix-axis-18 into 7b6fcbfe892341cb4ec7a672ec4f04c064d2e866 on AstroHuntsman:image_sim.

AnthonyHorton commented 5 years ago

Should also improve the tests, to ensure errors of this type will get caught in future. Currently the tests of the pixellated method in gunagala/tests/test_psf.py all use square images so that the order doesn't matter. Should make at least some of those rectangular, and check that the returned image is the right shape.

Worse, there are currently no tests of the image simulation features in gunagala/tests/test_imager.py yet. That's beyond the scope of this PR though, we'll fix that afterwards.

AnthonyHorton commented 5 years ago

Don't leave blank lines between the @pytest.mark.parameterize decorator and the test function declaration that follows it. The decorator is effectively part of the function definition so for readability reasons there should be no blank lines here.

Also, don't just use expected for the argument names for the expected values for all the parameterised test functions. Use more descriptive names, like expected_n_pix or expected_peak or image_size.

Note also that you can have as many arguments as you like, you don't have to use just two and start packing multiple values into them using tuples, and sometimes you might only need one if the only different between tests is which PSF you're testing.

AnthonyHorton commented 5 years ago

Also, try to get into the habit of running the tests on your own computer first so that you catch typos & other small errors before you commit & push.

bazkiaei commented 5 years ago

I am on it. It is my bad that forgot to run the test.

lspitler commented 5 years ago

@AnthonyHorton just checking where this is at. Were you going to sit down with @bazkiaei and finalise this? Is there any way I can help?

bazkiaei commented 5 years ago

@AnthonyHorton just checking where this is at. Were you going to sit down with @bazkiaei and finalise this? Is there any way I can help?

Maybe next Monday or Tuesday I go AAO to work on it?

AnthonyHorton commented 5 years ago

@lee Yes, would be good to get this merged. Just need to fix up a few of the tests. Shouldn't take long if @bazkiaei and I work on it together. We had discussed working at the AAO on it but Veloce intervened.

lspitler commented 5 years ago

@AnthonyHorton is there a good day for @bazkiaei to visit the AAO this week?