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

Use single date for agasc1p8 promotion #333

Merged
merged 2 commits into from
Aug 22, 2024
+15 −41
Merged

Use single date for agasc1p8 promotion #333

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
33f559e
Use single date for agasc1p8 promotion
javierggt Jul 15, 2024
9d952e6
set date_start_agasc1p8 to the AUG0524B loads starting time
javierggt Aug 20, 2024
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
34 changes: 7 additions & 27 deletions kadi/commands/observations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,37 +151,17 @@ def set_star_ids(aca: dict) -> None:
"""
from kadi.config import conf

date = aca["meta"]["date"]
if date < conf.date_start_agasc1p8_earliest:
# Always 1p7 before 2024-July-21 (before JUL2224 loads)
versions = ["1p7"]
elif date < conf.date_start_agasc1p8_latest:
# Could be 1p8 or 1p7 within 30 days later (uncertainty in promotion date)
versions = ["1p8", "1p7"]
else:
# Always 1p8 after 30 days after JUL2224
versions = ["1p8"]
version = "1p7" if aca["meta"]["date"] < conf.date_start_agasc1p8 else "1p8"

# Try allowed versions and stop on first success. If no success then issue warning.
# Try the AGASC version for that date. If no success then issue warning.
# Be aware that _set_star_ids works in place so the try/except is not atomic so the
# ``aca`` dict can be partially updated. This is not expected to be an issue in
# practice, and a warning is issue in any case.
err_star_id = None
for version in versions:
try:
agasc_file = agasc.get_agasc_filename(version=version)
except FileNotFoundError:
logger.warning(f"AGASC {version} file not found")
continue
try:
_set_star_ids(aca, agasc_file)
except StarIdentificationFailed as err:
err_star_id = err
else:
break
else:
# All versions failed, issue warning
logger.warning(str(err_star_id))
agasc_file = agasc.get_agasc_filename(version=version)
try:
_set_star_ids(aca, agasc_file)
except StarIdentificationFailed as err:
logger.warning(str(err))


def _set_star_ids(aca: dict, agasc_file: str) -> None:
Expand Down
11 changes: 5 additions & 6 deletions kadi/commands/tests/test_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,14 +841,14 @@ def test_get_starcats_each_year(year):
assert np.all(starcat["id"][ok] != -999)


def test_get_starcat_agasc1p8_then_1p7():
def test_get_starcat_only_agasc1p7():
"""
For obsid 2576, try AGASC 1.8 then fall back to 1.7 and show successful star
For obsids 3829 and 2576, try AGASC 1.7 only and show successful star
identification.
"""
with (
conf.set_temp("cache_starcats", False),
conf.set_temp("date_start_agasc1p8_earliest", "1994:001"),
conf.set_temp("date_start_agasc1p8", "2003:001"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the utility in moving the date start for agasc1p8 for this test anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test just verifies that the change took place. In other words: by setting an early date, the test will use 1p8, when in reality it used 1p7, and the test will fail.

We can rewrite the test and make it better, but somewhere we still need to check that the switch works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding "This test just verifies that the change took place." I don't see how the config set_temp to date_start_agasc1p8 for this test does that. I think, functionally, that is happening in
the other test, test_get_starcat_only_agasc1p8 .

Copy link
Member

Choose a reason for hiding this comment

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

@jeanconn - since the requested time frame in the line below is just before 2003:001, that means this is explicitly using AGASC 1.7 (as the test name implies).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but since the requested time frame is also before the standard config time for 1.8, the explicit set_temp doesn't show us anything.

Copy link
Member

Choose a reason for hiding this comment

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

It shows us that the code is not doing something silly like adding one day before comparing to the transition time. This is similar to other testing we've done of using a value that is purposely close to a limit.

Yes, not the strongest test in the world but the code is correctly testing both cases (before and after the limit). I think it is all good!

):
starcat = get_starcats(
"2002:365:18:00:00", "2002:365:19:00:00", scenario="flight"
Expand All @@ -865,10 +865,9 @@ def test_get_starcat_only_agasc1p8():
"""
with (
conf.set_temp("cache_starcats", False),
conf.set_temp("date_start_agasc1p8_earliest", "1994:001"),
conf.set_temp("date_start_agasc1p8_latest", "1994:002"),
conf.set_temp("date_start_agasc1p8", "1994:001"),
):
# Force AGASC 1.7 and show that star identification fails
# Force AGASC 1.8 and show that star identification fails
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we are going back and forth with this test, @taldcroft, did you see this change in the comment?

Just making sure I did not change it wrongly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I saw this and agreed.

with ska_helpers.utils.set_log_level(kadi.logger, "CRITICAL"):
starcats = get_starcats(
"2002:365:16:00:00", "2002:365:19:00:00", scenario="flight"
Expand Down
11 changes: 3 additions & 8 deletions kadi/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,9 @@ class Conf(ConfigNamespace):
False, "Include In-work command events that are not yet approved."
)

date_start_agasc1p8_earliest = ConfigItem(
"2024:210", # 2024-July-28
"Start date (earliest) for using AGASC 1.8 catalog.",
)

date_start_agasc1p8_latest = ConfigItem(
"2024:233", # 2024-July-28 + 23 days
"Start date (latest) for using AGASC 1.8 catalog.",
date_start_agasc1p8 = ConfigItem(
"2024:217:21:59:00", # 2024-08-04, start of AUG0524B loads
"Start date for using AGASC 1.8 catalog.",
)


Expand Down
Loading