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

Fix quickquasars targetid truth #457

Merged
merged 9 commits into from Dec 4, 2018
Merged

Conversation

londumas
Copy link
Contributor

@londumas londumas commented Dec 3, 2018

Fix issue #456:
- Correct the TARGETID of truth for all 4 HDUs
- Change z to Z for DLA

@londumas londumas requested a review from alxogm December 3, 2018 22:27
Copy link
Contributor

@alxogm alxogm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the deleted blank spaces and all other changes. Does the original bug is solved?

_meta['TARGETID'] = metadata['MOCKID']
_qsometa['TARGETID'] = metadata['MOCKID']
_meta['TARGETID'][:] = metadata['MOCKID']
_qsometa['TARGETID'][:] = metadata['MOCKID']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two lines are what fixes the original bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. Plus some other for DLA and BAL.

py/desisim/scripts/quickquasars.py Show resolved Hide resolved
Copy link
Contributor

@alxogm alxogm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably have to modify again the DLA part, once the final format in the transmission files is set. But I think all changes here are ok. Thanks @londumas

@londumas
Copy link
Contributor Author

londumas commented Dec 3, 2018

@alxogm, could you review again? I made some changes since the Lorentz smoothing was not stored in the truth file.
I decided to use DZ_FOG, DZ_SYS and DZ_STAT.
Thank for the help.

@londumas
Copy link
Contributor Author

londumas commented Dec 4, 2018

@andreufont and @alxogm, here is a summary of this PR.
I fixed a bug that was giving the wrong TARGETID to the truth file.
I changed the output of the truth file to give:

  • 'Z': final redshift
  • 'Z_INPUT': input redshift with RSD
  • 'DZ_FOG': delta z contribution of the Finger-Of-God
  • 'DZ_SYS': delta z contribution of the systematic shift
  • 'DZ_STAT': delta z contribution of the statistic shift (Lorentzian)
  • 'Z_NORSD': redshift without RSD

Do you both agree with these changes?

@alxogm
Copy link
Contributor

alxogm commented Dec 4, 2018

@londumas I'm only worried that we have been using TRUEZ and we'll get confused by changing to Z_INPUT. But I don't really know what is the most informative name, so I leave it to you and @andreufont to decide

@londumas
Copy link
Contributor Author

londumas commented Dec 4, 2018

@alxogm, is TRUEZ used by anyone for the moment?

@londumas
Copy link
Contributor Author

londumas commented Dec 4, 2018

I have no issue with changing back Z to TRUEZ.
It is TRUEZ_noFOG that I don't like.

Copy link
Contributor

@alxogm alxogm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alxogm, is TRUEZ used by anyone for the moment?

I think more recently by Luz, that is analyzing the BALs. I think Rodrigo and Andrea also uses it, and I do. At least. Of course we can adapt. But I think, we are getting confused with names. So I think we should have

'Z_NORSD': comes from transmission file and goes into truth file
'DZ_FOG': delta z contribution of the Finger-Of-God
'TRUEZ ': (Includes RSD and FOG) and goes into truth file.

This way we have replaced Z_noFOG by the DZ_FOG information.

I think it is clear that TRUEZ does not includes statistical or systematic shifts. You are also adding

'DZ_SYS': delta z contribution of the systematic shift
'DZ_STAT': delta z contribution of the statistic shift (Lorentzian)

And finally

'Z': Final redshift including RSD, FOG, and all shifts, that goes into the zbest file. Name is OK here.

##Adedd to write the truth file, includen metadata for DLA's and BALs
### Add a shift to the redshift, simulating the statistic imprecision of redrock
if args.gamma_kms_zfit:
log.info("Added zfit error with sigma {} to zbest".format(args.gamma_kms_zfit))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Added zfit error with sigma {} to zbest" please change sigma to gamma .

@londumas
Copy link
Contributor Author

londumas commented Dec 4, 2018

@alxogm, ok I understand better. I made the corrections.
Could you do a final review and give me the green light?

Copy link
Contributor

@alxogm alxogm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @londumas. I think I agree now.

@londumas londumas merged commit bc7e706 into master Dec 4, 2018
@londumas londumas deleted the fix_quickquasars_TARGETID_truth branch December 4, 2018 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants