-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simple Aperture Photometry: Gaussian1D fitting for radial profile #1409
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1409 +/- ##
==========================================
+ Coverage 85.13% 85.16% +0.03%
==========================================
Files 91 91
Lines 8321 8359 +38
==========================================
+ Hits 7084 7119 +35
- Misses 1237 1240 +3
Continue to review full report at Codecov.
|
You get the fitted model via API. So, are you saying you want the fitted FWHM to be displayed under GUI results? |
Looks like the previous plot didn't clear properly? I'll investigate. |
Yes. All of the fitted Gaussian parameters. |
|
It's not because no-recentering is happening. I think we should recenter, in which case the mean would be useless. |
By re-centering, do you mean moving the Subset to new center, or just in the number crunching? |
Number crunching, not moving the Subset. I expected the radial profile to always be centered on the source centroid. It would be very hard to manually position the Subset to be centered at the source centroid. |
Re: #1409 (comment) -- turns out the data points were unsorted, not related to plot clearing. FYI. |
106c6f1
to
99fd9df
Compare
I think I addressed most of the review comments but am waiting to hear back from @larrybradley about recentering. |
@@ -379,6 +417,15 @@ def vue_do_aper_phot(self, *args, **kwargs): | |||
f'{x:.4e} ({phot_table["aperture_sum_counts_err"][0]:.4e})'}) | |||
else: | |||
tmp.append({'function': key, 'result': str(x)}) | |||
# Also display fit results | |||
if fit_model is not None and isinstance(fit_model, Gaussian1D): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isinstance
check is future-proofing because it is just a matter of time before someone asks for Moffat.
99fd9df
to
fac8623
Compare
@larrybradley , I think I have addressed all your comments. Please re-review. Thanks! |
docs/imviz/plugins.rst
Outdated
to the radial profile, the last fitted model can be obtained by as follows. | ||
See :ref:`astropy:astropy-modeling` on how to manipulate the model:: | ||
|
||
my_gaussian1d = imviz.app.fitted_models['phot_radial_profile'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this conflict with anything else? We don't have the model fitting plugin for imviz, so probably won't be a problem unless we every include aperture photometry in a viz that does include model fitting, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What might be Cubeviz but none of that is even planned right now. Ideas welcome though. I am just trying to hook into existing machinery instead of creating even more API.
@pllim The radial plot titles all still say "from Subset center". Those should now say "from Source centroid". Also, I still don't see the output of the Gaussian1D fitted parameters. Where are those? The plots look nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to work well, and results clear from the table as would be expected when alternating between the different plot types.
jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue
Outdated
Show resolved
Hide resolved
@pllim To be clear, the fitted Gaussian1D parameters should be in a section separate from the |
instead of aperture center
[ci skip] Co-authored-by: Kyle Conroy <[email protected]>
and fix plot titles
like imexam PR 241
5a372c7
to
2da055a
Compare
gs = Gaussian1D(amplitude=y_max, mean=0, stddev=std, | ||
fixed={'mean': True, 'amplitude': True}, | ||
bounds={'amplitude': (y_max * 0.5, y_max)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hopefully now similar to spacetelescope/imexam#241 (comment) .
@larrybradley , please test this PR branch on some real data and let me know if this works as you expected. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pllim. This looks great.
The curve of growth plot title still says "from subset center" instead of "from source centroid."
@larrybradley , I didn't realize curve of growth was in scope too. I have pushed a commit for it. Please let me know if this is what you want. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran into a bug I think? When fitting for a subset works well as expected. But when I move the subset/redefine it on another star and click "Calculate", the chart doesn't update
Video of the above bug: ImvizExample.-.Jupyter.Notebook.-.Brave.2022-07-01.14-39-43.mp4 |
Yes, looks like the plugin is not listening to subset edit events. It is using cached info that is outdated. But this bug is not related to this PR, so can we open a new issue for that instead? Also, I think this plugin was written before @kecnry wrote the template mixin stuff, so I wonder if this bug will naturally go away if we refactor the plugin to use Kyle's mixin classes. |
Currently, to get around the bug, if you go to the "Aperture" dropdown and reselect the subset, it will update properly. |
p.p.s. When I wrote this originally, I might not have even realized you can go back and edit a subset after you finalized it. |
Agreed this bug was not introduced here, so can probably be considered out of scope and filed as a separate bug. But now that I did some digging, I'll report my findings here for when we do get back to it: aperture photometry does use the template mixin, but then also does its own caching which is why this is breaking. Since it is also using the change to subset to update defaults for things like the background annulus, we may want to think of ways to generalize that type of situation a little better. The quick-and-dirty fix would be to just trigger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the other bug I mentioned earlier being out of scope, this looks good! My only other suggestion is the ability to turn OFF the centroiding? I wonder if there are use cases we can't think of here where the user might want the raw result? Having an option to disable the centroiding might be a good option to provide? It would also clarify explicitly that centroiding is happening, beyond just the title (which some users may skim over)
I think this came up during one of the tag-ups. I am very hesitant to add more GUI to this plugin as it is already very crowded and Larry cannot think of a good reason why someone would not want centroiding. |
Perhaps I'm the minority here, in which case I'm happy to be outvoted, but I'll file my dissent here anyways :) I'm generally a fan of providing additional functionality regardless of whether we can or cannot think of a good reason. It allows our tools to be more responsive to usecases we can't think of that are outside of our perspective; though I can understand the UI concerns, though I feel like a small crosshair icon (like for "aim assist" in FPS games) wouldn't be too obstructive in this case 🤷 |
Maybe centroiding toggle can be a new issue too? 😸 |
I already briefly mentioned my thoughts on the switch on the call, but I'll add them here too in case we ever revisit. I think a switch serves two purposes:
(But I'm fine with postponing that decision/implementation) |
@kecnry said it much more eloquently than i did, but yes 💯 agree |
Since the original intent of this PR is approved twice, I am going to merge. I opened two follow up issues here:
Thanks for the thorough reviews! |
Description
This pull request is to enable Gaussian1D fitting for radial profile in the Simple Aperture Photometry plugin.
(Screenshots a bit outdated, but you get the idea.)
TODO
mean
at zero before fitting Gaussian1D. See imexamine - FWHM values in radial plot imexam#165 and fixes for #165, #238, #210, #205 imexam#241Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.CHANGES.rst
?