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

Add fids commanded state #313

Merged
merged 2 commits into from
Feb 1, 2024
Merged

Add fids commanded state #313

merged 2 commits into from
Feb 1, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jan 17, 2024

Description

This adds a kadi commanded state value fids which provides a set object with the fid IDs (1-14) that are commanded on.

In order to accomplish this I needed to make a change to the core states code. It was originally requiring that a transition callback had to be a method. In fact it just needs to be a callable, which then opens up the possibility of using a partial function as shown in this code.

Interface impacts

Testing

Unit tests

  • Mac
(razl) ➜  kadi git:(add-fids-state) git rev-parse HEAD                  
87d9b9792d3848812bf4289111963b8b6abded0c
(razl) ➜  kadi git:(add-fids-state) pytest                              
========================================================= test session starts =========================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 231 items                                                                                                                   

kadi/commands/tests/test_commands.py .....................................................................................      [ 36%]
kadi/commands/tests/test_states.py .......................x..............................................x..................... [ 76%]
..                                                                                                                              [ 77%]
kadi/commands/tests/test_validate.py ....................                                                                       [ 86%]
kadi/tests/test_events.py ..........                                                                                            [ 90%]
kadi/tests/test_occweb.py ......................                                                                                [100%]

============================================= 229 passed, 2 xfailed in 142.51s (0:02:22) ==============================================

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@taldcroft taldcroft requested a review from javierggt January 17, 2024 14:07
@taldcroft taldcroft force-pushed the deprecate-ska-load-dir branch from 2b977de to 5fcdf13 Compare January 18, 2024 20:18
@taldcroft taldcroft force-pushed the deprecate-ska-load-dir branch from 3724bc9 to 1d6011d Compare January 24, 2024 09:46
Base automatically changed from deprecate-ska-load-dir to master January 24, 2024 10:16
Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

I reviewed the code. It makes sense to me. I also went to check the CDB file referenced in the docstring to see that the commands there agreed. I also ran the tests.

@jeanconn
Copy link
Contributor

Overall LGTM, I note the first thing I did was try to compare the fid ids from kadi.commands.observations.get_starcats with these fid ids from states and those fid ids don't match up because get_starcats is also using the proseco ids 1-6 not the uniq numeric 1-14 ids seen in this code and backstop (printed in starcheck).

@taldcroft taldcroft merged commit d1b8982 into master Feb 1, 2024
4 checks passed
@taldcroft taldcroft deleted the add-fids-state branch February 1, 2024 09:58
@taldcroft
Copy link
Member Author

I just reviewed things a bit from the fid ID perspective and I think we are basically stuck with two systems that are both important and have use in different contexts. The 1-14 unique system relates to the fid commanding and the (detector, ID) system is deeply baked into fid positions and other characteristics (inherited from OFLS characteristics).

Going forward in razl the question is whether to display catalogs using the unique id or the per-detector id. It might make sense to work with the proseco per-detector value, but in fact with flexible output templates we can do both: per-detector with the modern razl template and unique with the starcheck template. See https://github.com/sot/razl/pull/8.

@jeanconn
Copy link
Contributor

jeanconn commented Feb 1, 2024

I disagree a little in that I think the "and the (detector, ID) system is deeply baked into fid positions" just became an interface style with proseco, doesn't really require public visibility, and we could update to use 1-14 everywhere a fid is displayed or requested by user. But I don't see a reason to bother at this point. We might want some breadcrumbs in documentation and a public translation function.

@taldcroft
Copy link
Member Author

I think the "and the (detector, ID) system is deeply baked into fid positions" just became an interface style with proseco

I was thinking of the OFLS characteristics, which have effectively used that system since before launch and then MATLAB, which inherited from that with their own characteristics.

we could update to use 1-14 everywhere a fid is displayed or requested by user. But I don't see a reason to bother at this point. We might want some breadcrumbs in documentation and a public translation function.

Agree with all that and I was independently thinking about a translation function.

This was referenced Mar 6, 2024
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