Skip to content
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

Merged
merged 3 commits into from
Jun 23, 2017

Conversation

yeganer
Copy link
Contributor

@yeganer yeganer commented Jun 21, 2017

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.

yeganer added 2 commits June 21, 2017 16:22
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.
Copy link
Contributor

@unoebauer unoebauer left a 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!

def spectrum_integrated(self):
return self.integrator.calculate_spectrum(
self.spectrum_frequency[:-1],
1000)
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

@wkerzendorf
Copy link
Member

@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.

Copy link

@talytha talytha left a 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!

@wkerzendorf wkerzendorf merged commit 9c0bd12 into formal_integral Jun 23, 2017
@yeganer yeganer deleted the fi_spectrum_integrated branch June 23, 2017 08:49
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.

4 participants