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

Allow specifying AGASC HDF5 file or latest proseco_agasc as default #387

Merged
merged 9 commits into from
Oct 3, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Sep 19, 2023

Description

This allows specifying the AGASC HDF5 that gets used in star selection via the AGASC_HDF5_FILE environment variable. If that is not set then it defaults to <AGASC_DIR>/proseco_agasc_<latest>.h5.

  • <AGASC_DIR> is the AGASC_DIR environment variable or $SKA/data/agasc.
  • The latest version of proseco_agasc is determined by the agasc package.

This PR requires sot/agasc#155.

As a bonus, I searched for os.environ in the code and documented all the environment variables that are used in proseco.

To do:

  • Add AGASC_DIR as a relevant env var.
  • Build the docs and confirm they look OK.

Interface impacts

None.

Testing

Unit tests

With the git repo for sot/agasc#155 in the PYTHONPATH:

  • Mac

Independent check of unit tests by Jean

  • Linux

Functional tests

Manual test of AGASC file

In my $SKA/data/agasc:

agasc1p7.h5
agasc1p8rc3.h5
agasc1p8rc4.h5
agasc1p8rc5.h5
agasc_healpix_1p7.h5
agasc_supplement.h5
miniagasc.h5
miniagasc_1p6.h5
miniagasc_1p7.h5
proseco_agasc_1p7.h5
proseco_agasc_1p8rc4.h5

Script:

from proseco.tests.test_common import STD_INFO
from proseco import get_aca_catalog
aca = get_aca_catalog(**STD_INFO)
print(aca.agasc_file)

Output:

/Users/aldcroft/ska/data/agasc/proseco_agasc_1p7.h5

Now make a temporary link from proseco_agasc_1p8rc4.h5 to proseco_agasc_1p8.h5
New output:

/Users/aldcroft/ska/data/agasc/proseco_agasc_1p8.h5

@jeanconn
Copy link
Contributor

With regard to other dependencies used in the matlab tools, it looks like in the weeds there's maybe just the ER optimization that isn't really used now https://github.com/sot/sparkles/blob/6349d0ff40d7b00ef7a9c7f20e8f2e6c58ea6e62/sparkles/find_er_catalog.py#L117

So a question about if sparkles should just ask proseco which agasc to use ?

@taldcroft
Copy link
Member Author

For that particular line you highlighted the miniagasc is fine because the goal is to find candidate acq/guide stars. The near-neighbor stars are all going to be filtered in the next lines of code.

So a question about if sparkles should just ask proseco which agasc to use ?

I suppose we could make a one line function in proseco:

def get_agasc_file():
    return  Path(os.environ.get("AGASC_HDF5_FILE", "proseco_agasc"))

"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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think from proseco SKA is only used for defining SKA/data but I haven't re-checked that.

@jeanconn
Copy link
Contributor

This looks good to me. I think we'd want some kind of explicit manual functional test that as applied with sot/agasc#155 that shows that, without AGASC_HDF5_FILE and AGASC_DIR set, that this code picks up proseco_agasc_* (so, proseco_agasc_1p7.h5 if that's in the $SKA/data/agasc directory and proseco_agasc_1p8.h5 if that's in $SKA/data/agasc directory, and proseco_agasc_1p8rc4.h5 is ignored).

@taldcroft
Copy link
Member Author

I think we'd want some kind of explicit manual functional test ...

Done, see Functional tests in the description.

@taldcroft taldcroft merged commit 85c79be into master Oct 3, 2023
2 checks passed
@taldcroft taldcroft deleted the agasc1p8 branch October 3, 2023 15:13
@javierggt javierggt mentioned this pull request Dec 5, 2023
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.

3 participants