Skip to content

Conversation

@lspitler
Copy link
Member

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:

  • fixtures can no longer be called directly
  • at some point, we renamed the fixtures but didn't update the arg to the unit tests

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: astropy/astropy#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 lspitler requested a review from AnthonyHorton August 31, 2020 12:13
@lspitler
Copy link
Member Author

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.

astropy/astropy#9293

@lspitler
Copy link
Member Author

lspitler commented Sep 1, 2020

Submitted issue and hopefully a PR over at astropy: astropy/astropy#10695

This was referenced Oct 20, 2020
@lspitler
Copy link
Member Author

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

@lspitler
Copy link
Member Author

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
Copy link
Member Author

@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 lspitler marked this pull request as draft October 27, 2020 01:15
@lspitler
Copy link
Member Author

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
Copy link
Member Author

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

@lspitler lspitler closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant