search

AstroHuntsman / gunagala

4 stars 9 forks source link

Improving psf_pytest.py #26 #28

Closed bazkiaei closed 5 years ago

bazkiaei commented 5 years ago

This PR is just to improve the test_psf.py appearance and efficiency. Although there is some other problems that I should try to solve, but as @wtgee has advised, I try to keep this PR small and just for small issues.

Right know, my goal at this PR is to reach to the main face of test_psf.py, specially to use @pytest.mark.parametrize.

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.1%) to 87.656% when pulling 704113ed12d027060fb611f6f91e5e413aa7432a on bazkiaei:improve-pytest-#26 into b86466994a264f262f1512e3851adfff9b20a54d on AstroHuntsman:image_sim.

AnthonyHorton commented 5 years ago

@bazkiaei You've made the pull request to the wrong branch. Your improve-pytest-#26 branch was based off the image-sim branch so you should be making the pull request into that branch, but this PR is into develop. Consequently it's pulling in all the changes from the image-sim branch along with your changes to the tests.

lspitler commented 5 years ago

@bazkiaei where are we at on this one? Part of the problem is that as @wtgee says there is no summary of the Issue you're trying to address here.

Also I'm not sure what "pytes" means? Is it a typo?

bazkiaei commented 5 years ago

@lspitler Well I today found out that I have merged image_sim branch to this branch so whatever I was changing, were resulting to a change to the image_sim branch. So I spend much time to find out how to undo the merge that I had done. before that I had made some changes to test_psf.py that had to wait for branches corrections to be committed. I committed them now but seems there are some conflicts with changes that I have made so that is my next activity on this PR.

@AnthonyHorton I know that there are some conflicts with what I have committed, but I will appreciate if you take a look and check what I have done is close to what you mentioned in your last comment. I hope that I have understood the points you mentioned.

lspitler commented 5 years ago

@bazkiaei can you remove .pytest_cache/v/cache/lastfailed from this branch? I don't think it should be in the repo.

@AnthonyHorton might be offline for many weeks. Can you describe what the aim of this PR is? Is it adding parameterise? How does it relate to #26 and #27 ? We need more info to help out with this.

bazkiaei commented 5 years ago

@lspitler We started this repo to improve test_psf.py both in appearance and efficiency. To do that we had started to use pytest.mark.parametrize and using that lead us to issue #26 but I will start another PR for that as soon as I know the shape of the test is good and the only needed change is that of issue #26 .

lspitler commented 5 years ago

The aim of this review (which is ready) is to address @AnthonyHorton suggestion to do the following:

The thing is, you have an image_size parameter but you're just giving it the same value each time, so you're not really using it.

If you're going to have an image_size parameter you should be using it to pass different image sizes, which would enable you to combine the test_pixellated_square, test_pixellated_rectangles, etc. tests into one.

AnthonyHorton commented 5 years ago

Need to fix the four failing tests before merging, though. Think the cause of the problem is that you've got astropy Quantitys on the left side of the comparisons but a plain float inside the pytest.approx() call.

Note, dividing a Quantity by its unit as you do here does not convert it to a basic numeric type. The result is still a Quantity, it's just a dimensionless one with unit dimensionless_unscaled. The correct way to get the value of a Quantity in a given unit is to use quantity_variable.to(desired_unit).value instead.