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

DM-17029: Update LoadReferenceObjectsTask to output fluxes in nanojansky #281

Merged
merged 1 commit into from Apr 5, 2019

Conversation

parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Mar 8, 2019

No description provided.

Copy link
Contributor

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

Only one comment, but I think it is an important one.

refMagErrArr = abMagErrFromFluxErr(refFluxErrArr, refFluxArr)
refMagArr = u.Quantity(refFluxArr, u.nJy).to_value(u.ABmag)
# HACK convert to Jy until we have a replacement for this (DM-16903)
refMagErrArr = abMagErrFromFluxErr(refFluxErrArr*1e-9, refFluxArr*1e-9)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this be internal to abMagErrFromFluxErr. That would make implementing DM-16903 easier since you wouldn't have to look for every place abMagErrFromFluxErr is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn on this one. There are fortunately very few uses of these methods, but would be an API change. I don't know if we can include it under RFC-575? I suppose with that RFC, all flux units dealt with in the stack will be nJy, so maybe it is appropriate to change these functions now?

It looks like these two magErr<->fluxErr functions are only used in a handful of places in the stack, so maybe it wouldn't affect all that much?

Copy link
Contributor

Choose a reason for hiding this comment

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

I trust your judgement on this one. I didn't appreciate the API change argument until you voiced it. I agree there are few enough instances of those functions and it is easy enough to grep that it shouldn't be a big deal.

Copy link
Contributor

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

I understand your hesitance. Do what you think is best.

This should make the Calib->PhotoCalib transition in DM-10156 easier, I hope.

Still need to hack around abMagErrFromFluxErr (see DM-16903).
@parejkoj parejkoj merged commit 2cfc051 into master Apr 5, 2019
@timj timj deleted the tickets/DM-17029 branch February 18, 2021 15:50
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.

None yet

2 participants