added magnitude estimation (formerly known as mag-stats) #48
Conversation
agasc/agasc.py
Outdated
| return t[0] | ||
|
|
||
|
|
||
| def get_stars(ids, agasc_file=None, dates=None, fix_color1=True): | ||
| def get_stars(ids, agasc_file=None, dates=None, fix_color1=True, supplement_file=None): |
| STARS_OBS_NP = None | ||
|
|
||
|
|
||
| def _load_startcat_commands(tstop=None): |
|
@javierggt - I'm working on a commit to implement my suggestions, so don't worry about this. |
|
The current dtype of the entries in the I would suggest instead: |
|
@javierggt - I committed a new approach for doing AGASC queries that also includes tests. I also just realized that having the |
|
I'm good with that. I will start using this. |
|
The existing file |
0022822 to
b543863
Compare
| :return: dict | ||
| dictionary with stats | ||
| """ | ||
| min_mag_obs_err = 0.3 |
There was a problem hiding this comment.
Maybe this is the bit that needs to be 0.03?
There was a problem hiding this comment.
With a review of the testing and light review of the agasc/agasc.py changes, this seems fine to me. I do wonder:
- would we be better off with downstream fixes instead of the old and new hacks on COLOR1?
- do we still want the color1=1.5 acquisition grid model for a star that (before this code changed its value to 1.49) had COLOR1=1.5? I wasn't sure how much of that model selection was based on larger mag error and how much was based on redder stars (not that we can tease those apart completely) I'm also not sure how different the models in there are at this point.
|
No we do not want the color1=1.5 model for stars that have an estimated mag. The ONLY reason for the distinct model is that the mag_errs for color1=1.5 stars follow a completely different distribution. Once we know the mag of a star then it is no longer in that weird distribution, instead it follows a gaussian distribution (more-or-less) with MAG_ACA_ERR. The red-ness of a star is not relevant once you have a mag. |
What do you mean by downstream fixes? |
|
By downstream fixes, I meant changing the handling in proseco sparkles starcheck chandra_aca to be more explicit instead of implicit... basically to remove anything that is using COLOR1=1.5 or COLOR1=0.7 checks. But I suppose with the current agasc and supplement, we'd be left with a bit of a mess of logic around RSV1, COLOR2, MAG_CATID, unless we added a new column that somewhat defined "does this need extra mag error padding or special handing in the acq model". I understand that this hack in the agasc code to rewrite COLOR1 works fine with code in the wild like: https://github.com/sot/proseco/blob/7ad393513387cbe8e08396793ecc5c144f6ace12/proseco/core.py#L1130 just also wondering if that code in proseco really belongs in the agasc module. Maybe my point in the end is that this agasc change should also kick off an audit/sanity check of how we're using COLOR1 in our code. |
|
I guess downstream would mean changes in proseco. I added the 'color' column to the supplement, which in principle allows you to specify color in the supplement as well, but I can remove that today since it is not used. |
Yes, color does not belong in the supplement since we do not measure it with the ACA. |
|
removed it. |
|
Indeed we would need a new column which basically says that a star behaves like a color=0.7 or color=1.5 star. Given all the code out there (and not just in a few core packages, but probably dozens of little scripts and whatnot) that uses this existing convention, I don't see a big advantage when the end result will be exactly the same. Doing the upstream single-source fix in the I agree we might want to migrate that fix in proseco up to agasc. |
|
Sounds good. Thanks for having the conversation about it. |
|
|
||
| - ``MAG_ACA`` and ``MAG_ACA_ERR`` are set according to the supplement. | ||
| - ``MAG_CATID`` (mag catalog ID) is set to ``MAG_CATID_SUPPLEMENT`` (128). | ||
| - If COLOR1 is 0.7 or 1.5 then it is changed to 0.69 or 1.49 respectively. |
There was a problem hiding this comment.
Not sure if we might want a clarifying comment indicating "if the COLOR1 hasn't already been updated based on APASS" or some such.
Description
This is a modified version of what is in the mica mag-stats branch. I renamed most modules and functions.
Testing
Fixes #