-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
[TEP005][QOL] Add MontecarloRunner.spectrum_integrated #750
Conversation
This is a shortcut for obtaining an integrated spectrum for the wavelength grid defined in the configuration file. If incompatible configuration settings are detected, an IntegrationError is raised.
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.
Apart from my comment concerning the hard-coded p-resolution, this looks very good!
tardis/montecarlo/base.py
Outdated
def spectrum_integrated(self): | ||
return self.integrator.calculate_spectrum( | ||
self.spectrum_frequency[:-1], | ||
1000) |
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 hardcoded 1000 is the resolution of the p-grid, right? If so, I think we should make this also a configuration variable.
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 name would you suggest for such a configuration option?
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.
something like impact_parameter_bins
@unoebauer I agree that it would be nice to have a config option for the p-values, so I would say that we put this as an item in the project todo - but that we merge this PR. |
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.
For me it also looks good!
This is a shortcut for obtaining an integrated spectrum for the
wavelength grid defined in the configuration file. If incompatible
configuration settings are detected, an IntegrationError is raised.