HRC state updates for 24V, 15V (from SCS-134), HRC_I and HRC_S

jskrist commented 1 year ago

Description

Updated states.py:

Account for the HRC 15V power on state changing due to the 215PCAON within SCS 134.
Add a new hrc_24v state which triggers off of 224PCAON and 224PCAOF
Fix the hrc_i and hrc_s states to go OFF based on 2IMHVOF and 2SPHVOF, respectively.

Fixes #279, #280

Interface impacts

Adds SCS-134 as a load command that will change the hrc_15v state to ON. Since SCS-134 has never been previously commanded this has no impact.

Testing

Updated test_hrc_states() to include the HRC 24v state data
Added a test of the new SCS-134 functionality using commands from the FEB0623T test loads.

Unit tests

[ ] Windows (JK)
[x] Mac (TA)
[x] Linux (JC)

Functional tests

Plan

Test with the updated acis_thermal_check https://github.com/acisops/acis_thermal_check/pull/64 as noted https://github.com/acisops/acis_thermal_check/pull/64#issuecomment-1456661999. That was run by Dan.

JC overplotted the FOT model run outputs and the last SOT model run outputs (provided in email "HRC LR cea_check and Ska3").

DEC0522P_2ceahvpt JAN3023U_2ceahvpt FEB0623T_2ceahvpt

(plots from https://icxc.cfa.harvard.edu/aspect/test_review_outputs/kadi-pr281/kadi_cea_review.html)

jskrist commented 1 year ago

I don't know how to mock the kadi commands in order to insert the new SCS commanding to test that branch of the code explicitly, but would be happy to add the test if someone can help me figure out how to.

taldcroft commented 1 year ago

@jskrist - thanks for getting started on this. There shouldn't be a need for the inner code changes you made to add in a transition due to SCS-134. You can see how this is handled in the current version with some new commits.

FYI I used this notebook as a helper in the development process. It illustrates some tricks that help: https://gist.github.com/taldcroft/c17bbaef6ae2c9cf759f9a7dc4472580

jskrist commented 1 year ago

@taldcroft thanks for updating the code and getting to this so quickly. I see from your changes how to mock the commands for testing this.

How do these new changes handle historical SCS-134 calls (that weren't HRC 15v on command states? Sorry if it's obvious from your notebook, I'm on my phone this morning and the notebook it's rendering well.

taldcroft commented 1 year ago

BTW, the 24V HRC state is a fine thing to have, but it is not physically needed for the HRC thermal modeling. See https://github.com/acisops/acis_thermal_check/pull/64#issuecomment-1454767061.

taldcroft commented 1 year ago

@jskrist - SCS 134 has never been activated from the load commanding:

cmds = get_cmds()  # Gets all 1.5M commands in the archive
ok = (cmds['tlmsid'] == "COACTSX") & (cmds["coacts1"] == 134)
cmds[ok]
# Empty table

So we're fine. During the first 22 years (pre-B-side anomaly), even if there had been SCS-134 activations in the loads it would have been benign because it would be turning on the 15V that was already on.

jskrist commented 1 year ago

@taldcroft great, thanks for checking.

matthewdahmer commented 1 year ago

@taldcroft The 24v state definitely does have an effect on the modeled HRC temperature. I agree that it is very difficult to fit, but in reality it does affect temperature, and if left on inadvertently would easily push the cea temperature above the observing guideline limit.

matthewdahmer commented 1 year ago

@taldcroft I see what you meant about the 24v not affecting the model output when fitting, and agree. The time span the 24v is on is just way too short to have a real effect. That being said if it is kept on (such as by mistake), it would affect temperature.

taldcroft commented 1 year ago

FYI @jeanconn is going to do the review of this since I've contributed commits now.

jeanconn commented 1 year ago

I think the current implementation does not have the interface impacts called out in the PR top description. Correct?

taldcroft commented 1 year ago

@jeanconn - I've updated the description.

taldcroft commented 1 year ago

I also just force-pushed a commit update (changed commit message) to force GitHub actions to restart. It's now green.

jeanconn commented 1 year ago

This looks good to me, assuming that the only kadi-state-impacting thing in SCS134 is a command to turn on hrc_15v that happens at the time of activation. I'm waiting for more updates to https://github.com/acisops/acis_thermal_check/pull/64 and that functional testing before approving.

taldcroft commented 1 year ago

@jeanconn - the only command in SCS-134 is the HRC 15V on.

sot / kadi