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

Improve implementation of characteristics from ACA xija model #386

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Aug 15, 2023

Description

This improves the implementation of proseco.characteristics (commonly ACA in the code) so that the characteristics that are derived from the chandra_models aca_spec.json thermal model spec always reflect the correct values. This applies even when environment variables like CHANDRA_MODELS_REPO_DIR are changed during program execution.

Previously the values were cached based on the selected repo and version at the first time the characteristics were accessed. The test which changed illustrates how things work more nicely now, in particular no need for explicitly dealing with caching.

Thanks to @jeanconn in #385 (review) for pointing out this possibility.

Interface impacts

None.

Testing

Unit tests

  • Mac

Independent check of unit tests by Jean

  • Linux

Functional tests

No functional testing.

@taldcroft taldcroft requested a review from jeanconn August 15, 2023 19:30
@taldcroft taldcroft mentioned this pull request Aug 15, 2023
2 tasks
@jeanconn
Copy link
Contributor

I think this is great!

@jeanconn
Copy link
Contributor

But I ran into an issue from the last PR #385 that I'm surprised passes tests:

(ska3) spark:proseco jean$ ipython
Python 3.10.8 | packaged by conda-forge | (main, Nov 22 2022, 08:27:35) [Clang 14.0.6 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.8.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import proseco.tests.test_catalog
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[1], line 1
----> 1 import proseco.tests.test_catalog

File ~/git/proseco/proseco/tests/test_catalog.py:21
     18 from .test_common import DARK40, OBS_INFO, STD_INFO, mod_std_info
     20 # Ensure all plotting is to a non-interactive backend
---> 21 matplotlib.pyplot.switch_backend("agg")
     23 HAS_SC_ARCHIVE = Path(mica.starcheck.starcheck.FILES["data_root"]).exists()
     24 TEST_COLS = "slot idx id type sz yang zang dim res halfw".split()

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/matplotlib/_api/__init__.py:224, in caching_module_getattr.<locals>.__getattr__(name)
    222 if name in props:
    223     return props[name].__get__(instance)
--> 224 raise AttributeError(
    225     f"module {cls.__module__!r} has no attribute {name!r}")

AttributeError: module 'matplotlib' has no attribute 'pyplot'

@jeanconn
Copy link
Contributor

Granted, I guess it doesn't really matter because the tests aren't meant to be run that way anyway... they need their pytest magics.

@taldcroft
Copy link
Member Author

taldcroft commented Aug 17, 2023

I wasn't sure if that "auto-import" of pyplot would work when I first wrote the code, but I thought that getting through pytest was a sufficient demonstration. Huh, magics indeed. Anyway, fixed.

@taldcroft taldcroft merged commit 769b0aa into master Aug 17, 2023
@taldcroft taldcroft deleted the use-chandra-models-cache branch August 17, 2023 13:36
@javierggt javierggt mentioned this pull request Sep 6, 2023
@javierggt javierggt mentioned this pull request Sep 18, 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.

2 participants