Update for SCS-106 change and new power commands #41

Gregg140 · 2021-09-30T16:43:44Z

Description

Recently, SCS-106 was updated to leave 3 FEPs on rather than no FEPs on.
SCS-106 is run as part of the SCS-107 sequence.

Therefore thermal model history assembly requires that this change be reflected
in any shutdown: both science-only shutdowns as well as Full Stops. Also, the
ACIS Team recently added new power commands which have to be handled by Backstop History.

These were the drivers for the release of this version of Backstop History.
Several changes had been underway for some time and are incorporated in this release.

Testing

[x ] Passes unit tests on MacOS, linux, Windows (at least one required)
[x ] Functional testing

Fixes #

acis_thermal_check/state_builder.py

acis_thermal_check/utils.py

jzuhone · 2021-10-01T17:58:51Z

I'm getting some regression test failures--I'll be checking them to see what's causing them.

Gregg140 · 2021-10-04T12:51:55Z

I'm getting some regression test failures--I'll be checking them to see what's causing them.

You should be getting errors whenever an SCS-107 was run. This is the crux of the change - SCS-107 calls SCS-106 and 106 now leaves 3 FEP's on, whereas before it left no FEP's on.

So the DPA temps should be very different as well as FEP count. Here is an example(circa 355:10):

acis_thermal_check/state_builder.py

jzuhone · 2021-10-06T20:00:41Z

acis_thermal_check/state_builder.py

        self.bs_cmds = bs_cmds

-        # Clip commands to tbegin
-        bs_cmds = bs_cmds[bs_cmds['date'] > tbegin]


So in the old version we were clipping commands to tbegin. Based on your changes, do we not want to do this anymore, or is in being done in backstop_history now?

Backstop history was supposed to do that but the ~30 minute backoff caused it to backchain one
extra Continuity load and, without the clipping, that caused the kadi_states.get_states to return the time-disrupted result.

Putting the line back in and clipping the bs_cmds seems to have fixed it.

I will re-run the regression tests and check the FEP plot to be sure.

Thanks

Ok--sounds good. Probably a good idea in the future to revisit the -5 element backoff and find something to do that maybe makes more sense (or at least is easier to understand)

jzuhone · 2021-10-07T02:52:30Z

I have an update that I'd like to add to this PR, which should be a no-op--it only clears up the role of the run-start argument and gets rid of some confusing code.

I've added it to my own branch--you can see it here:

jzuhone@25adbd8

If you want to add it, let me know and we can just push this commit here.

Gregg140 · 2021-10-07T11:16:34Z

Sure lets include this update in this release. I'll need a line or two for the release notes.

jazan12 · 2021-10-13T15:28:32Z

acis_thermal_check/main.py

@@ -1067,10 +1072,10 @@ def make_validation_plots(self, tlm, model_spec, outdir, run_start):
        f.write(quant_table)
        f.close()

-        # If run_start is specified this is likely for regression testing
+        # If self.write_pickle is specified this is likely for regression testing


Check: is "specified" or is True?

is True, yes

Actually "specified" is a much better comment.

Comments are not supposed to replicate/repeat the code: the reader can see how the if statement works. Comments are there to tell the reader what the intent is....to explain what's going on so that the reader does not have to waste time figuring out what's going on.

As a simple example, suppose you are incrementing x:

x +=1

The following is a useless comment:

# Increment x
x +=1

Anyone can see that x is being incremented. The comment provides no additional information.
The reader does not know why you are doing that.

But this is a more useful comment:

# Point to the next item in the star chart list.
x+=1

"specified" is better than True. But it can be made better still. I changed it to:

# self.write_pickle is set to the value of the --run-start command # line argument; either a DOY date string or None (if the argument # was not specified). If a DOY date, this model run is likely for regression # testing or other debugging. In that case write out the full # predicted and telemetered dataset as a pickle.

Anyone trying to figure out what we did now knows how self.write_pickle gets set, why it is set
and the intent of the block of code.

jazan12 · 2021-10-13T15:31:07Z

acis_thermal_check/state_builder.py

@@ -45,7 +45,7 @@ def get_validation_states(self, datestart, datestop):
        Parameters
        ----------
        datestart : string
-            The start date to grab states afterward.
+            The start date to grab states afterward.builder_class


missing carriage return before builder_class?

No just some text that wasn't there in the original. It's been removed.

jazan12 · 2021-10-13T15:33:32Z

acis_thermal_check/state_builder.py

-        states from the review load backstop file and all the
-        states between the latest telemetry data and the beginning
+        commands from the review load backstop file and all the
+        commands between the latest telemetry data and the beginning
        of that review load backstop file.

        The Review Backstop commands already obtained.
        Telemtry from 21 days back to the latest in Ska obtained.


Telemetry misspelled

jazan12 · 2021-10-13T15:34:16Z

acis_thermal_check/state_builder.py

-        # Convert backstop commands from a list of dict to a CommandTable and
-        # store in self.
-        bs_cmds = kadi.commands.CommandTable(bs_cmds)
+        # Ask Backstop History to assemble the history for this loa


"loa" --> "load"

jazan12

Looks ok to me Gregg. I flagged some small typos / potential wording issues in your description, but nothing in the code raised any flags with me.

jazan12 · 2021-10-15T16:52:39Z

Hey Gregg, Thanks for your explanation. I'm reacting to this: self.write_pickle = args.run_start is not None from which I infer that write_pickle is True/False. In this case, I would think a value could be *False* but to me that's still specified, as opposed to empty / None which is what I would take from "unspecified". Now, you have: # self.write_pickle is set to the value of the --run-start command but I don't think that's the case - it's T/F, right?

…

On Fri, Oct 15, 2021 at 11:28 AM Gregg140 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In acis_thermal_check/main.py <#41 (comment)> : > @@ -1067,10 +1072,10 @@ def make_validation_plots(self, tlm, model_spec, outdir, run_start): f.write(quant_table) f.close() - # If run_start is specified this is likely for regression testing + # If self.write_pickle is specified this is likely for regression testing Actually "specified" is a much better comment. Comments are not supposed to replicate/repeat the code: the reader can see how the if statement works. Comments are there to tell the reader what the intent is....to explain what's going on so that the reader does not have to waste time figuring out what's going on. As a simple example, suppose you are incrementing x: x +=1 The following is a useless comment: *# Increment x* x +=1 Anyone can see that x is being incremented. The comment provides no additional information. The reader does not know why you are doing that. But this is a more useful comment: *# Point to the next item in the star chart list.* x+=1 "specified" is better than True. But it can be made better still. I changed it to: # self.write_pickle is set to the value of the --run-start command # line argument; either a DOY date string or None (if the argument # was not specified). If a DOY date, this model run is likely for regression # testing or other debugging. In that case write out the full # predicted and telemetered dataset as a pickle. Anyone trying to figure out what we did now knows how self.write_pickle gets set, why it is set and the intent of the block of code. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#41 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACN5TGKCGRCVWNBBGHPCQT3UHBCAXANCNFSM5FCYKPCQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Update for SCS-106 change and new power commands

6ce6d8e

Gregg140 requested review from jzuhone, cegrant and jazan12 September 30, 2021 16:44

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

Typos in comments fixed

ed946cc

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

jzuhone reviewed Sep 30, 2021

View reviewed changes

acis_thermal_check/utils.py Outdated Show resolved Hide resolved

Typos fixed and spaces taken out of verbose lines

40ddd9b

jzuhone reviewed Oct 6, 2021

View reviewed changes

acis_thermal_check/state_builder.py Outdated Show resolved Hide resolved

jzuhone reviewed Oct 6, 2021

View reviewed changes

Make the role of the run_start argument much more clear

25adbd8

Included changes John made to make operation of run-start more clear

5cdc0dd

jazan12 reviewed Oct 13, 2021

View reviewed changes

jazan12 approved these changes Oct 13, 2021

View reviewed changes

Fixed typo items and one comment that Jack found

ab929cd

Modified one comment

726f74a

jzuhone approved these changes Oct 18, 2021

View reviewed changes

Gregg140 merged commit 65639e9 into master Oct 18, 2021

This was referenced Oct 18, 2021

Update acisops/backstop_history to 1.5.0 sot/skare3#719

Closed

Update acisops/acis_thermal_check to v3.7.0 sot/skare3#720

Closed

2021.14 sot/skare3#723

Merged

Gregg140 mentioned this pull request Oct 21, 2021

Write .hist files to --outdir directory #42

Merged

1 task

This was referenced Oct 22, 2021

Update acisops/acis_thermal_check to 3.7.0 sot/skare3#724

Closed

Update acisops/acis_thermal_check to 3.7.1 sot/skare3#725

Closed

Update acisops/backstop_history to 1.5.1 sot/skare3#726

Closed

javierggt mentioned this pull request Jun 17, 2022

2022.5 (closed) sot/skare3#850

Closed

javierggt mentioned this pull request Aug 3, 2022

2022.5 sot/skare3#899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for SCS-106 change and new power commands #41

Update for SCS-106 change and new power commands #41

Gregg140 commented Sep 30, 2021

jzuhone commented Oct 1, 2021

Gregg140 commented Oct 4, 2021

jzuhone Oct 6, 2021

Gregg140 Oct 6, 2021

jzuhone Oct 7, 2021

jzuhone commented Oct 7, 2021

Gregg140 commented Oct 7, 2021

jazan12 Oct 13, 2021

jzuhone Oct 13, 2021

Gregg140 Oct 15, 2021

jazan12 Oct 13, 2021

Gregg140 Oct 15, 2021

jazan12 Oct 13, 2021

Gregg140 Oct 15, 2021

jazan12 Oct 13, 2021

Gregg140 Oct 15, 2021

jazan12 left a comment

jazan12 commented Oct 15, 2021 via email

Update for SCS-106 change and new power commands #41

Update for SCS-106 change and new power commands #41

Conversation

Gregg140 commented Sep 30, 2021

Description

Testing

jzuhone commented Oct 1, 2021

Gregg140 commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzuhone commented Oct 7, 2021

Gregg140 commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazan12 left a comment

Choose a reason for hiding this comment

jazan12 commented Oct 15, 2021 via email