Add fids commanded state

taldcroft commented 5 months ago

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

[x] 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
<!-- Describe and document results of any functional tests, otherwise leave the text below -->
No functional testing.

jeanconn commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

sot / kadi