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

bugfix: correct (1+z) factor in equivalent width #1878

Merged
merged 1 commit into from Oct 21, 2022
Merged

bugfix: correct (1+z) factor in equivalent width #1878

merged 1 commit into from Oct 21, 2022

Conversation

araichoor
Copy link
Contributor

This PR corrects a bug in emlinefit.emlines_gaussfit() for the equivalent width measurement.
It fixes #1876.

@moustakas
Copy link
Member

I double-checked by measuring the EWs of some strong lines in a couple spectra by hand and the fix in this PR (and an equivalent one in FastSpecFit) does indeed address the underlying bug.

@moustakas
Copy link
Member

@araichoor please update the change log and then you should be able to merge.

@araichoor
Copy link
Contributor Author

thanks for all the quick checks!

just, what is the change log?
do you mean doc/changes.rst ?
if so, I think the desispec procedure is to update it when creating a tag.

@moustakas
Copy link
Member

just, what is the change log?
do you mean doc/changes.rst ?
if so, I think the desispec procedure is to update it when creating a tag.

I'm not aware of a change in procedure. Generally every PR should include an appropriate line in doc/changes.rst under the "unreleased" version number so that when we go to make a tag we don't have to hunt back through the various merged PR to figure out which ones came after the previous tag.

That said, looking back through the last many PRs, it doesn't look like this procedure has been followed. So I guess I'm wrong!

@araichoor
Copy link
Contributor Author

I let @sbailey decide here :)

@sbailey
Copy link
Contributor

sbailey commented Oct 21, 2022

Yes, every PR should eventually be reflected by an entry in doc/changes.rst. However, we often have multiple simultaneous PRs going on in desispec and making the updates prior to merging causes unnecessary merge conflicts. What we should be doing is adding the entry immediately after merging, but in practice it usually piles up and then I fill in a bunch of entries at once just before making a new tag.

I'm about to make one of those big "update doc/changes.rst from all the recent PRs" updates, so I'll take care of this one too.

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.

emlinefit 1+z factors
3 participants