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

Apply temperature-dependent drift model offset to fid positions #388

Merged
merged 38 commits into from
Dec 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0c86b27
Implement use of an initial fid drift correction
jeanconn Oct 13, 2023
f39da1e
Update to use new get_fid_offset function in chandra_aca
jeanconn Oct 17, 2023
c29342b
Update PROSECO_ENABLE_FID_OFFSET to tri-state
jeanconn Oct 29, 2023
e52fe59
Update warning text
jeanconn Oct 29, 2023
550e0ef
Remove unnecessary monkeypatch
jeanconn Nov 11, 2023
b4bfa31
Update comment
jeanconn Nov 11, 2023
56d76a1
Update docs to include new env var
jeanconn Nov 11, 2023
d62bc35
Add fid offset functional testing and position check notebooks
jeanconn Nov 30, 2023
5595c4c
Add a couple of comments to the fid review notebook
jeanconn Nov 30, 2023
af53856
Update docs/index.rst
jeanconn Dec 5, 2023
e97c593
Update proseco/fid.py in fid position docs
jeanconn Dec 5, 2023
b37bd20
Update proseco/fid.py ValueError text
jeanconn Dec 5, 2023
a7325d3
Update proseco/fid.py get_fid_position docs
jeanconn Dec 5, 2023
eed0380
Simplify PROSECO_ENABLE_FID_OFFSET handling
jeanconn Dec 5, 2023
d27e70a
Move supplement disable to conftest.py
jeanconn Dec 7, 2023
a688570
Update warning test with newer text
jeanconn Dec 7, 2023
ceb3c26
Don't rely on the fixture value for offset tests
jeanconn Dec 7, 2023
6ed6d97
Split off the throw-error fid offset tests
jeanconn Dec 7, 2023
6d560f8
Update some equality tests to actually test that
jeanconn Dec 7, 2023
421b3c9
Update get_fid_catalog call in add_fake_stars_from_fid
jeanconn Dec 8, 2023
0be47e8
Require acquisition temperature in FidTable
jeanconn Dec 8, 2023
27071d1
Remove fixture to disable fid offsets
jeanconn Dec 8, 2023
8e25a0b
Supply acq and guide temperature in test_common
jeanconn Dec 8, 2023
e9a3e16
Disable fid offsets in one acq test
jeanconn Dec 8, 2023
2b0e3a1
Update fid positions in exp/regress values to offset positions
jeanconn Dec 8, 2023
7a299d0
Remove unused imports
jeanconn Dec 8, 2023
b5fcc08
Use a FIDS test fixture
jeanconn Dec 8, 2023
ef377fa
Rename notebooks
jeanconn Dec 8, 2023
3761755
Rerun validation notebook
jeanconn Dec 8, 2023
1535aef
Set t_ccd_acq from t_ccd kwarg
jeanconn Dec 12, 2023
9036aa0
Add a test fixture to disable fid offsets
jeanconn Dec 12, 2023
be17b85
Disable fid offsets in two tests
jeanconn Dec 12, 2023
989143d
Make some new t_ccd kwarg/attr tests
jeanconn Dec 12, 2023
e8a5529
Rerun validate notebook
jeanconn Dec 12, 2023
440b2da
Restore previous comment
jeanconn Dec 12, 2023
0b63ec0
Use disable_fid_offsets fixture in an acq test
jeanconn Dec 12, 2023
5c2b26f
Revert change to add_fake_stars_from_fid
jeanconn Dec 13, 2023
d86e281
Some tiny cleanup
taldcroft Dec 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 201 additions & 0 deletions analysis/pr388-backstop-vs-proseco-star-positions.ipynb

Large diffs are not rendered by default.

3,593 changes: 3,593 additions & 0 deletions analysis/pr388-validate-drift-model-fid-positions.ipynb

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -311,13 +311,19 @@ The following environment variables are used by proseco:
If this is a relative path then it is relative to ``<default_agasc_dir>``.
- ``AGASC_SUPPLEMENT_ENABLED``: set to ``"False"`` to disable using the AGASC
supplement. This is for testing and should not be used in production.
- ``PROSECO_ENABLE_FID_OFFSET``: controls application of time and temperature dependent fid
light position offsets (from the ACA drift model) in :ref:`~proseco.fid.get_fid_positions`:
- Not set: apply offsets if time and temperature are provided (as is done in ``proseco`` fid
selection since version 5.12.0)
- ``"True"``: require that time and temperature be provided and apply offsets.
- ``"False"``: do not apply offsets (typically used in regression testing not production).
- ``PROSECO_IGNORE_MAXAGS_CONSTRAINTS``: if set then do not update ``maxmag`` in the
catalog to prevent search hits clipping.
- ``PROSECO_OR_IMAGE_SIZE``: override the default OR image size of 8x8. Can be one of
"4", "6", or "8".
- ``PROSECO_PRINT_OBC_CAT``: if set then create and print a debug catalog while doing
catalog merging.
- ``SKA``: root directory for Ska3 runtime environment
- ``SKA``: root directory that contains 'data' directory

API docs
--------
Expand Down
44 changes: 41 additions & 3 deletions proseco/fid.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@
Get a catalog of fid lights.
"""

import os
import weakref

import numpy as np
from chandra_aca.drift import get_fid_offset
from chandra_aca.transform import yagzag_to_pixels
from cxotime import CxoTimeLike

from . import characteristics as ACA
from . import characteristics_acq as ACQ
Expand Down Expand Up @@ -56,7 +59,6 @@ def get_fid_catalog(obsid=0, **kwargs):
return empty_fids

fids.set_stars(acqs=fids.acqs)

fids.cand_fids = fids.get_fid_candidates()

# Set list of available fid_set's, accounting for n_fid and cand_fids.
Expand Down Expand Up @@ -94,6 +96,7 @@ class FidTable(ACACatalogTable):
"detector",
"sim_offset",
"focus_offset",
"t_ccd_acq",
taldcroft marked this conversation as resolved.
Show resolved Hide resolved
"t_ccd_guide",
"date",
"dither_acq",
Expand Down Expand Up @@ -128,6 +131,7 @@ def t_ccd(self):
@t_ccd.setter
def t_ccd(self, value):
self.t_ccd_guide = value
self.t_ccd_acq = value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In more detailed review and testing, this was the change needed so that "get_fid_catalog" will mostly do the right thing if just a t_ccd is supplied. In that case the temperature is assigned to both t_ccd_guide and t_ccd_acq and fid offsets will be applied based on t_ccd_acq by default. This was already the default behavior for get_aca_catalog, but not for get_fid_catalog.

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable.


def set_fid_set(self, fid_ids):
if len(self) > 0:
Expand Down Expand Up @@ -316,7 +320,11 @@ def get_fid_candidates(self):
Result is updating self.cand_fids.
"""
yang, zang = get_fid_positions(
self.detector, self.focus_offset, self.sim_offset
self.detector,
self.focus_offset,
self.sim_offset,
t_ccd=self.t_ccd_acq,
date=self.date,
)
row, col = yagzag_to_pixels(yang, zang, allow_bad=True)
ids = np.arange(len(yang), dtype=np.int64) + 1 # E.g. 1 to 6 for ACIS
Expand Down Expand Up @@ -472,7 +480,13 @@ def set_spoilers_score(self, fid):
)


def get_fid_positions(detector, focus_offset, sim_offset):
def get_fid_positions(
detector: str,
focus_offset: float,
sim_offset: float,
t_ccd: float | None = None,
date: CxoTimeLike | None = None,
) -> tuple:
"""Calculate the fid light positions for all fids for ``detector``.

This is adapted from the Matlab
Expand All @@ -492,9 +506,14 @@ def get_fid_positions(detector, focus_offset, sim_offset):
yfid=-SIfield.fidpos(:,1)/(SIfield.focallength-xshift);
zfid=-zshift/(SIfield.focallength-xshift);

This function also applies a temperature-dependent fid offset if ``t_ccd`` and ``date``
are supplied and the ``PROSECO_ENABLE_FID_OFFSET`` env var is ``"True"`` or not set.

:param detector: 'ACIS-S' | 'ACIS-I' | 'HRC-S' | 'HRC-I'
:param focus_offset: SIM focus offset [steps]
:param sim_offset: SIM translation offset from nominal [steps]
:param t_ccd: CCD temperature (C)
:param date: date (CxoTime compatible)

:returns: yang, zang where each is a np.array of angles [arcsec]
"""
Expand All @@ -518,4 +537,23 @@ def get_fid_positions(detector, focus_offset, sim_offset):

yang, zang = np.degrees(yfid) * 3600, np.degrees(zfid) * 3600

enable_fid_offset_env = os.environ.get("PROSECO_ENABLE_FID_OFFSET")
if enable_fid_offset_env not in ("True", "False", None):
raise ValueError(
f'PROSECO_ENABLE_FID_OFFSET env var must be either "True", "False", or not set, '
f"got {enable_fid_offset_env}"
)

has_tccd_and_date = t_ccd is not None and date is not None

if enable_fid_offset_env == "True" and not has_tccd_and_date:
raise ValueError(
"t_ccd_acq and date must be provided if PROSECO_ENABLE_FID_OFFSET is 'True'"
)

if has_tccd_and_date and enable_fid_offset_env != "False":
dy, dz = get_fid_offset(date, t_ccd)
yang += dy
zang += dz

return yang, zang
11 changes: 11 additions & 0 deletions proseco/tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
import pytest
import agasc
from agasc import get_agasc_filename


@pytest.fixture()
def disable_fid_offsets(monkeypatch):
monkeypatch.setenv("PROSECO_ENABLE_FID_OFFSET", "False")


@pytest.fixture(autouse=True)
def use_fixed_chandra_models(monkeypatch):
monkeypatch.setenv("CHANDRA_MODELS_DEFAULT_VERSION", "3.48")


@pytest.fixture(autouse=True)
jeanconn marked this conversation as resolved.
Show resolved Hide resolved
def disable_agasc_supplement(monkeypatch):
monkeypatch.setenv(agasc.SUPPLEMENT_ENABLED_ENV, "False")


# By default test with the latest AGASC version available including release candidates
@pytest.fixture(autouse=True)
def proseco_agasc_rc(monkeypatch):
Expand Down
7 changes: 2 additions & 5 deletions proseco/tests/test_acq.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import os
from pathlib import Path

import agasc
import numpy as np
import pytest
from chandra_aca import star_probs
Expand Down Expand Up @@ -34,9 +33,6 @@
CACHE = {} # Cache stuff for speed
TEST_COLS = ("idx", "slot", "id", "yang", "zang", "halfw")

# Do not use the AGASC supplement in testing by default since mags can change
os.environ[agasc.SUPPLEMENT_ENABLED_ENV] = "False"


def calc_p_brightest(acq, box_size, stars, dark, man_err=0, dither=20, bgd=0):
"""
Expand Down Expand Up @@ -846,11 +842,12 @@ def get_dark_stars_simple(box_size_thresh, dither):
return dark, stars


def test_acq_fid_catalog_probs_low_level():
def test_acq_fid_catalog_probs_low_level(disable_fid_offsets):
"""
Low-level tests of machinery to handle different fid light sets within
acquisition probabilities.
"""

# Put an acq star at an offset from fid light id=2 such that for a search
# box size larger than box_size_thresh, that star will be spoiled. This
# uses the equation in FidTable.spoils().
Expand Down
104 changes: 87 additions & 17 deletions proseco/tests/test_catalog.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
import copy
import os
import pickle
from pathlib import Path

Expand All @@ -24,9 +23,6 @@
HAS_SC_ARCHIVE = Path(mica.starcheck.starcheck.FILES["data_root"]).exists()
TEST_COLS = "slot idx id type sz yang zang dim res halfw".split()

# Do not use the AGASC supplement in testing by default since mags can change
os.environ[agasc.SUPPLEMENT_ENABLED_ENV] = "False"

HAS_MAG_SUPPLEMENT = len(agasc.get_supplement_table("mags")) > 0


Expand Down Expand Up @@ -100,12 +96,13 @@ def test_get_aca_catalog_20603(proseco_agasc_1p7):
img_size_guide=6,
raise_exc=True,
)

# Expected 2 fids, 4 guide, 7 acq
exp = [
"slot idx id type sz yang zang dim res halfw",
"---- --- --------- ---- --- -------- -------- --- --- -----",
" 0 1 4 FID 8x8 2140.23 166.63 1 1 25",
" 1 2 5 FID 8x8 -1826.28 160.17 1 1 25",
" 0 1 4 FID 8x8 2136.87 163.42 1 1 25",
" 1 2 5 FID 8x8 -1829.63 156.96 1 1 25",
" 2 3 116791824 BOT 6x6 622.00 -953.60 28 1 160",
" 3 4 40114416 BOT 6x6 394.22 1204.43 24 1 140",
" 4 5 40112304 BOT 6x6 -1644.35 2032.47 12 1 80",
Expand Down Expand Up @@ -145,9 +142,9 @@ def test_get_aca_catalog_20259(proseco_agasc_1p7):
exp = [
"slot idx id type sz yang zang dim res halfw",
"---- --- --------- ---- --- -------- -------- --- --- -----",
" 0 1 1 FID 8x8 -1175.03 -468.23 1 1 25",
" 1 2 2 FID 8x8 1224.70 -460.93 1 1 25",
" 2 3 3 FID 8x8 -1177.69 561.30 1 1 25",
" 0 1 1 FID 8x8 -1176.88 -470.85 1 1 25",
" 1 2 2 FID 8x8 1222.86 -463.55 1 1 25",
" 2 3 3 FID 8x8 -1179.54 558.68 1 1 25",
" 3 4 896009152 BOT 6x6 1693.39 217.92 16 1 100",
" 4 5 897712576 BOT 6x6 -1099.95 2140.23 12 1 80",
" 5 6 897717296 BOT 6x6 932.58 1227.48 12 1 80",
Expand Down Expand Up @@ -568,10 +565,84 @@ def test_call_args_attr():
"obsid": 0,
"optimize": False,
"sim_offset": 0,
"t_ccd": -11,
"t_ccd_acq": -11,
"t_ccd_guide": -11,
}


def test_t_ccd_attr():
Copy link
Member

Choose a reason for hiding this comment

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

I like the new tests!

"""
Test that a t_ccd kwarg is set to all of the ACACatalogTable attrs
"""

aca_args = STD_INFO.copy()
for kw in ["t_ccd", "t_ccd_acq", "t_ccd_guide"]:
if kw in aca_args:
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, a one-line way to do this is aca_args.pop(kw, None).

Copy link
Member

Choose a reason for hiding this comment

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

And if it ever comes up again, we could update mod_info() (or whatever is that function) to allow for deleting kwargs via mod_info(delete_kwargs=("t_ccd", "t_ccd_acq", "t_ccd_guide")).

del aca_args[kw]

t_ccd = -11
aca_args["t_ccd"] = t_ccd
aca = get_aca_catalog(**aca_args)
assert aca.t_ccd_acq == t_ccd
assert aca.t_ccd_guide == t_ccd
assert aca.acqs.t_ccd == t_ccd
assert aca.guides.t_ccd == t_ccd
assert aca.fids.t_ccd == t_ccd
assert aca.fids.t_ccd_acq == t_ccd
assert aca.fids.t_ccd_guide == t_ccd


def test_t_ccd_acq_attr():
"""
Test that a t_ccd and t_ccd_acq kwarg are set to all of the expected ACACatalogTable attrs
"""

aca_args = STD_INFO.copy()
for kw in ["t_ccd", "t_ccd_acq", "t_ccd_guide"]:
if kw in aca_args:
del aca_args[kw]

t_ccd = -11
t_ccd_acq = -16
aca_args["t_ccd"] = t_ccd
aca_args["t_ccd_acq"] = t_ccd_acq
aca = get_aca_catalog(**aca_args)
assert aca.t_ccd_acq == t_ccd_acq
assert aca.t_ccd_guide == t_ccd
assert aca.acqs.t_ccd == t_ccd_acq
assert aca.guides.t_ccd == t_ccd
assert aca.fids.t_ccd == t_ccd
assert aca.fids.t_ccd_acq == t_ccd_acq
assert aca.fids.t_ccd_guide == t_ccd


def test_t_ccd_multi_attr():
"""
Test assignments if t_ccd, t_ccd_acq, and t_ccd_guide supplied as kwargs
"""

aca_args = STD_INFO.copy()
for kw in ["t_ccd", "t_ccd_acq", "t_ccd_guide"]:
if kw in aca_args:
del aca_args[kw]

t_ccd = -11
t_ccd_acq = -16
t_ccd_guide = -6
aca_args["t_ccd"] = t_ccd
aca_args["t_ccd_acq"] = t_ccd_acq
aca_args["t_ccd_guide"] = t_ccd_guide
aca = get_aca_catalog(**aca_args)
assert aca.t_ccd == t_ccd_guide
assert aca.t_ccd_acq == t_ccd_acq
assert aca.t_ccd_guide == t_ccd_guide
assert aca.acqs.t_ccd == t_ccd_acq
assert aca.guides.t_ccd == t_ccd_guide
assert aca.fids.t_ccd == t_ccd_guide
assert aca.fids.t_ccd_acq == t_ccd_acq
assert aca.fids.t_ccd_guide == t_ccd_guide


def test_bad_obsid():
# Expects this to be starcheck catalog
aca = get_aca_catalog(obsid="blah blah", raise_exc=False)
Expand Down Expand Up @@ -679,7 +750,7 @@ def test_monitors_and_target_offset_args():
assert aca.target_offset is target_offset


def test_reject_column_spoilers():
def test_reject_column_spoilers(disable_fid_offsets):
"""
Test that column spoiler handling is correct for guide, acq and fid selection.
Also tests not selecting stars that are too bright.
Expand Down Expand Up @@ -734,8 +805,8 @@ def offset(id, drow, dcol, dmag, rmult=1):

def test_dark_property():
"""
Test that in the case of a common t_ccd, all the dark current maps are
actually the same object.
Test that appropriate temperatures are applied to acq, fid, guide
dark maps.

:return: None
"""
Expand All @@ -744,7 +815,6 @@ def test_dark_property():
assert aca.dark is getattr(aca, attr).dark

kwargs = STD_INFO.copy()
del kwargs["t_ccd"]
kwargs["t_ccd_acq"] = -12.5
kwargs["t_ccd_guide"] = -11.5
aca = get_aca_catalog(**kwargs)
Expand All @@ -766,9 +836,9 @@ def test_dense_star_field_regress(proseco_agasc_1p7):
exp = [
"slot idx id type sz yang zang dim res halfw mag ",
"---- --- ---------- ---- --- -------- -------- --- --- ----- -----",
" 0 1 3 FID 8x8 40.01 -1871.10 1 1 25 7.00",
" 1 2 4 FID 8x8 2140.23 166.63 1 1 25 7.00",
" 2 3 5 FID 8x8 -1826.28 160.17 1 1 25 7.00",
" 0 1 3 FID 8x8 35.52 -1874.72 1 1 25 7.00",
" 1 2 4 FID 8x8 2135.73 163.01 1 1 25 7.00",
" 2 3 5 FID 8x8 -1830.77 156.55 1 1 25 7.00",
" 3 4 1130899056 BOT 8x8 2386.83 -1808.51 28 1 160 6.24",
" 4 5 1130889232 BOT 8x8 -251.98 -1971.97 28 1 160 6.99",
" 5 6 1130893664 BOT 8x8 1530.07 -2149.38 28 1 160 7.62",
Expand Down
3 changes: 2 additions & 1 deletion proseco/tests/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
date="2018:001",
n_guide=5,
n_fid=3,
t_ccd=-11,
t_ccd_acq=-11,
t_ccd_guide=-11,
jeanconn marked this conversation as resolved.
Show resolved Hide resolved
man_angle=90,
dither=8.0,
)
Expand Down
Loading
Loading