-
Notifications
You must be signed in to change notification settings - Fork 3
Create a model that chooses a random SED to use #364
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
==========================================
+ Coverage 91.98% 92.12% +0.14%
==========================================
Files 50 51 +1
Lines 3494 3532 +38
==========================================
+ Hits 3214 3254 +40
+ Misses 280 278 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Click here to view all benchmarks. |
| self.sed_values[model_ind][1, :], | ||
| left=0.0, | ||
| right=0.0, | ||
| ) |
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.
Will there be any extrapolation handled by PhysicalModel or should extrapolation be enabled here?
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 short answer is that this should be handed by PhysicalModel.
The longer answer is that I realized this won't work for randomly selected models because minwave and maxwave might be different. I updated the PR to support this in both the StatisSEDSource and in a few other models. So extrapolation should work better throughout the code now.
|
|
||
| def minwave(self, graph_state=None): | ||
| """Get the minimum wavelength of the model. For additive models, this is | ||
| the minimum wavelength of all sources. |
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.
I'm not sure about this. This may cause some artifacts if one of the model is truncated instead of physically having shorter coverage. If we'd like to use this approach, I would do the maximum of all minwaves for minwave and minimum of all maxwaves for maxwave. Or we should do extrapolations for each source indivicually so they end up having the same min/maxwaves.
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.
It is already doing the actual extrapolations one on each source individually (if wavelength extrapolation is turned on). And then adding them. This allows blended objects at radically different redshifts or a host model with a very different wavelength range than the source.
I changed minwave and maxwave to mark the bounds of the intersection of the wavelengths as you suggested. But the extrapolation will still work on a per-source basis.
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.
But the extrapolations start at the shared min/maxwaves instead of individual ones. Am I understanding it correctly?
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.
No. The extrapolations will start at each individual model's min/maxwaves, because the additive model's _evaluate_single() function calls the individual source's _evaluate_single() functions. Each of those functions determines whether (and how) to do extrapolation based only on that source's information, including its specific min/max wavelengths.
So if we query 1000.0 for an additive model where the host model wavelength starts at 500.0 and the source model starts at 2000.0, we will use the sum of the actual value from the host model and the extrapolated value from the source.
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 sounds great! But is this min/maxwave() in additive model used anywhere then?
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.
It is not currently. I considered having the functions for the additive model throw errors, but thought we might want to use it in some analysis. For example we could add a line to plots to show extrapolated vs none extrapolated regions (in which case the intersection approach you proposed and we now use would make the most sense). I can still remove the functions if you'd prefer.
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.
hmmm.. I see now it could be useful to keep it. Can we return a list with min/max for each source if it's not called in other places? That will be more helpful for the plotting case you mentioned.
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.
Done
Closes #360
Uses that SED as a constant over all time steps.