-
Notifications
You must be signed in to change notification settings - Fork 3
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
HRC state updates for 24V, 15V (from SCS-134), HRC_I and HRC_S #281
Conversation
moving HRC 15V state to scs
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. |
@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: |
@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. |
BTW, the 24V HRC state is a fine thing to have, but it is not physically needed for the HRC thermal modeling. See acisops/acis_thermal_check#64 (comment). |
@jskrist - SCS 134 has never been activated from the load commanding:
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. |
@taldcroft great, thanks for checking. |
@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. |
@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. |
FYI @jeanconn is going to do the review of this since I've contributed commits now. |
I think the current implementation does not have the interface impacts called out in the PR top description. Correct? |
@jeanconn - I've updated the description. |
994ab63
to
4412dbd
Compare
I also just force-pushed a commit update (changed commit message) to force GitHub actions to restart. It's now green. |
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 acisops/acis_thermal_check#64 and that functional testing before approving. |
@jeanconn - the only command in SCS-134 is the HRC 15V on. |
Description
Updated states.py:
215PCAON
within SCS 134.hrc_24v
state which triggers off of224PCAON
and224PCAOF
hrc_i
andhrc_s
states to goOFF
based on2IMHVOF
and2SPHVOF
, respectively.Fixes #279, #280
Interface impacts
hrc_15v
state toON
. Since SCS-134 has never been previously commanded this has no impact.Testing
Unit tests
Functional tests
Plan
Test with the updated
acis_thermal_check
acisops/acis_thermal_check#64 as noted acisops/acis_thermal_check#64 (comment). 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").
(plots from https://icxc.cfa.harvard.edu/aspect/test_review_outputs/kadi-pr281/kadi_cea_review.html)