Rearrange photometry settings and add object for photometry options #272
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #272 +/- ##
==========================================
+ Coverage 69.74% 70.22% +0.47%
==========================================
Files 24 24
Lines 2938 2992 +54
==========================================
+ Hits 2049 2101 +52
- Misses 889 891 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Mostly good (like usual). I caught a couple of issues. Two big ones:
- Do we need more tests of PhotometryOptions class?
- Do we need to provide a function to the user to set the PhotometryOptions?
Other than that, it's all the small documentation issues I usually see. I do think we should be clearer to users when listing photometry_options as a parameter for the Single image and Multi-image photometry who the user can set those parameters. I would also consider listing them in the documentation for the photometry routines.
| Camera, | ||
| Observatory, | ||
| PhotometryApertures, | ||
| PhotometryOptions, |
There was a problem hiding this comment.
Not here, but I looked up PhotometryOptions in settings/models.py and noticed a typo in line 424 of that file "constrols" --> "controls".
stellarphot/photometry/photometry.py
Outdated
| noise for each observation. If ``False``, only the Poisson noise from | ||
| the source and the sky will be included. | ||
| photometry_options : `stellarphot.settings.PhotometryOptions` | ||
| Several options for the details of performing the photometry. |
There was a problem hiding this comment.
Do we want to clarify how a user should set those options in the docs here?
stellarphot/photometry/photometry.py
Outdated
| noise for each observation. If ``False``, only the Poisson noise from | ||
| the source and the sky will be included. | ||
| photometry_options : `stellarphot.settings.PhotometryOptions` | ||
| Several options for the details of performing the photometry. |
There was a problem hiding this comment.
Again, do we want to clarify how a user should set those options in the docs here?
| # use_coordinates="pixel". In the former case, the expected | ||
| # result is the test below. In the latter case, the expected | ||
| # result is that either obs_avg_net_cnts is nan or the difference | ||
| # is bigger than the expected_deviation. |
There was a problem hiding this comment.
Am I misreading this? You say you expect the different is BIGGER than the expected_deviation, but below (line 556) you assert it is smaller, no?
Also, just for my sake, can you clarify why the noise will be different in sky coords versus pixel coord?
There was a problem hiding this comment.
Two things
- I rearranged the comments to make clear what the passing condition is in each case.
- I think there is a bug somewhere, either
- In
list_of_imageor in the shifting stuff that calls. The reason I say that is I think we should be able to use either pixel or sky coordinates and have this pass. In practice, the image first image is fine but the second gets negative net counts and bad fwhms. - Maybe in
multi_image_photometrysince that treats the first image different than the rest.
- In
I'll track down what exactly the error is later this morning, I hope.
There was a problem hiding this comment.
You say you expect the different is BIGGER than the expected_deviation, but below (line 556) you assert it is smaller, no?
oops, now fixed
| @@ -288,6 +304,16 @@ def test_observatory_lat_long_as_float(): | |||
| assert obs == Observatory(**DEFAULT_OBSERVATORY_SETTINGS) | |||
|
|
|||
|
|
|||
| def test_photometry_settings_negative_shift_tolerance(): | |||
There was a problem hiding this comment.
Should we test the other parameters of PhotometryOptions?
Thanks 😀
I don't think so because it gets all of the tests that are here in addition for the test for the one parameter that has a constraint.
I'm adding an example of creating one to the |
|
This was a comment inline but I'm pulling it out here too so it doesn't get overlooked. It refers to this code.
I'll track down what exactly the error is later this morning, I hope. |
|
Good news, there is no bug. The reason the test in question doesn't find sources when |
mwcraig
force-pushed
the
rearrange-photometry-settings
branch
from
4ae2b25 to
d7da0fb
Compare
Also fixed an import in a test.
mwcraig
force-pushed
the
rearrange-photometry-settings
branch
from
d7da0fb to
f63f2cc
Compare
| # see the line where found_sources is defined. | ||
| # | ||
| # However, the images are shifted with respect to each other by | ||
| # list_of_fakes, so there are no long stars at those positions in the |
There was a problem hiding this comment.
"no long" --> "no longer"?
There was a problem hiding this comment.
🤣 I wish we could take care of collimation issues in the software! Will fix in the next batch of changes.
This is another fairly big one. It does a few things:
single_image_photometryandmulti_image_photometryso the order is the same, with keywords specific to each function at the end of each signature.PhotometryOptionsobject that includesshift_toleranceand all of the keyword options that were common tosingle_image_photometryandmulti_image_photometry. Also adds tests of the new object.shift_toleranceand the keyword options that were common to both.multi_image_photometrywere not being passed through tosingle_image_photometry😀@JuanCab -- my step will be to use these to turn the photometry functions into a class. I'm going to start building that based on this, but can of course modify in response to suggestions here.
If you can review this one by early next week that would be great. The next one, "class"-ifying photometry, will be much less extensive than this I think.
Closes #149