search

AstroHuntsman / gunagala

4 stars 9 forks source link

Unit test issues #39

Closed lspitler closed 3 years ago

lspitler commented 4 years ago

Bringing a few commits from #38 to here as they are seperate to that PR. @AnthonyHorton I need some input on this one.

First few commits are minor updates I needed to run unit tests in test_psf.py:

I think these are minor, though I needed them to run tests.

The main problem is that astropy's discretize_oversample_1D/2D was recently updated to use np.linspace, for good reason: https://github.com/astropy/astropy/pull/9293

Trouble is that np.linspace now requires the arg num to be an integer: https://numpy.org/doc/stable/reference/generated/numpy.linspace.html

This wasn't an issue before as discretize_oversample_1D/2D used np.arange which accepted different inputs just fine from the gunagala.psf functions used: https://numpy.org/doc/stable/reference/generated/numpy.arange.html

@AnthonyHorton can you suggest a way forward?

My last commit now forces integer x_range and y_range, but I don't think this is the right way to do this.

lspitler commented 4 years ago

Actually sleeping on this, I think a simple modification in astropy's discretize_oversample_1D/2D would solve this problem. They just need to force an int() in the num arg to linspace... I might submit an Issue/PR to astropy.

https://github.com/astropy/astropy/pull/9293

lspitler commented 4 years ago

Submitted issue and hopefully a PR over at astropy: https://github.com/astropy/astropy/issues/10695

lspitler commented 3 years ago

The astropy fix has now been released: https://github.com/astropy/astropy/releases/tag/v4.0.2

lspitler commented 3 years ago

One set of error is that np.where doesn't carry the units through anymore:

Fix is to convert: signal = np.where(saturated, 0 * u.electron / u.pixel, signal) to: signal = np.where(saturated, 0 , signal) * u.electron / u.pixel

lspitler commented 3 years ago

@AnthonyHorton @danjampro can you do a review on this PR? It is ready to merge.

Travis is failing on certain builds, but we'll reconfiguring the test environment when #38 is merged so we can address it over there.

lspitler commented 3 years ago

Pausing this effort as it seems #38 works just fine without these changes. This might imply my incorrect usage of outdated astropy versions during my tests.

lspitler commented 3 years ago

Closing as #38 tests (without the changes in this PR) work just fine. @lspitler likely was using a dated testing environment.