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.

Sign up for GitHub

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

Jump to bottom

Make ACA penalty limit optional #384

Merged
merged 2 commits into from
Aug 14, 2023
Merged

Make ACA penalty limit optional #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e76a96c
WIP: make ACA penalty limit optional
taldcroft Aug 11, 2023
348320e
Improve description of chandra_models_version
taldcroft Aug 11, 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
2 changes: 1 addition & 1 deletion proseco/catalog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ def get_effective_t_ccd(t_ccd, t_ccd_penalty_limit=None):
if t_ccd_penalty_limit is None
else t_ccd_penalty_limit
)
if t_ccd > t_limit:
if t_limit is not None and t_ccd > t_limit:
return t_ccd + 1 + (t_ccd - t_limit)
else:
return t_ccd
Expand Down
41 changes: 19 additions & 22 deletions proseco/characteristics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import json

import agasc
from ska_helpers import chandra_models
from ska_helpers.utils import LazyDict

CCD = {
Expand Down Expand Up @@ -74,13 +77,12 @@ def _load_bad_star_set():

bad_star_set = LazyDict(_load_bad_star_set)

# Version of chandra_models to use for the ACA xija model from which the
# planning and penalty limits are extracted. Default of None means use the
# current flight-approved version. For testing or other rare circumstances this
# can be overridden prior to calling get_aca_catalog(). This module variable
# gets set to the actual chandra_models version when the ACA attributes are
# first accessed.
chandra_models_version = None
# READ-ONLY variable which gives the version of chandra_models being use for the ACA
# xija model from which the planning and penalty limits are extracted. This module
# variable is set on demand to the chandra_models repo version. To select a specific
# version set the CHANDRA_MODELS_DEFAULT_VERSION environment variable.
#
# chandra_models_version

# The next two characteristics are lazily defined to ensure import succeeds.

Expand All @@ -93,7 +95,11 @@ def _load_bad_star_set():

def __getattr__(name):
"""Lazily define module attributes for the ACA planning and penalty limits"""
if name in ("aca_t_ccd_penalty_limit", "aca_t_ccd_planning_limit"):
if name in (
"chandra_models_version",
"aca_t_ccd_penalty_limit",
"aca_t_ccd_planning_limit",
):
_set_aca_limits()
return globals()[name]
else:
Expand All @@ -103,25 +109,16 @@ def __getattr__(name):
def _set_aca_limits():
"""Set global variables for ACA thermal planning and penalty limits"""
global chandra_models_version
from xija.get_model_spec import get_xija_model_spec

spec, chandra_models_version = get_xija_model_spec(
"aca", version=chandra_models_version
)
spec_txt, info = chandra_models.get_data("chandra_models/xija/aca/aca_spec.json")
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 assumed the plan was to update get_xija_model_spec file to use chandra_models.get_data to have a unified approach via that strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but that is out of scope for this PR. Also, using the direct interface in ska_helpers makes more sense here since there is no real use of xija itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes -- though for me I was bringing it up because starcheck was getting the ACA model version and the limits from proseco here via characteristics.py, but getting the aca model spec with get_xija_model_spec. That leads to a disconnect in that application due to the interface change here (in scope). It probably makes sense for starcheck to either join proseco in using ska_helpers.chandra_models.get_data to get the aca model or to stop using proseco.characteristics as a convenient source of limit reading. Or update get_xija_model_spec to use ska_helpers.chandra_models.get_data.

Copy link
Member

Choose a reason for hiding this comment

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

I should mention that get_xija_model_spec() does not work on FOT Windows machines. it returns an error about not being able to read from remote repository when executing the line repo = git.Repo.clone_from(repo_path, repo_path_local). I have confirmed that this occurs from the python environment directly, and is not related to the pyexec interface. I think we don't run into this issue now only because MATLAB provides all the necessary inputs, so this code doesn't run when called from MATLAB. Except it did in my testing of removing this parameter (before I started using this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Though James I was working under the impression that part of the problem with get_xija_model_spec was that right now the windows environment had a broken git .

Copy link
Member

Choose a reason for hiding this comment

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

That could be the root of the issue. I'm not sure exactly what or how the git is broken in the python environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

We had previously seen what we guessed was a conda bug making a broken git. It was a non-issue because the windows environment wasn't using git for anything but will hope it is fixed upstream or will work around it for the next testing release (if that is coming into play).

spec = json.loads(spec_txt)

chandra_models_version = info["version"]

names = ("aca_t_ccd_penalty_limit", "aca_t_ccd_planning_limit")
spec_names = ("planning.penalty.high", "planning.warning.high")
for name, spec_name in zip(names, spec_names):
try:
limit = spec["limits"]["aacccdpt"][spec_name]
except KeyError:
raise KeyError(
f"unable to find ['limits']['aacccdpt']['{spec_name}'] "
"in the ACA xija model in "
f"chandra_models version {chandra_models_version}."
)
else:
globals()[name] = limit
globals()[name] = spec["limits"]["aacccdpt"].get(spec_name)


# Make sure module-level `dir()` includes the lazy attributes.
Expand Down
Loading