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

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

Merged
merged 5 commits into from
Oct 18, 2021

Conversation

Gregg140
Copy link
Contributor

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)
  • Functional testing

Fixes #

@@ -330,7 +246,7 @@ def get_continuity_file_info(self, oflsdir):

# Read the first line...the path to the continuity load. The
# continuity path in the file is hardwired to a /data/acis path,
# independent of user-specified `oflsdir` (e.g.
# independent of user-specified `oflsdir` (e.g.BackstopHistory.py
Copy link
Member

Choose a reason for hiding this comment

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

This got copied in somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


# Convert tbegin to cxcsec if necessary
if type(tbegin) is str:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if type(tbegin) is str:
if isinstance(tbegin, str):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced.

event_list = self.Find_Events_Between_Dates(self.master_ToFC, self.end_event_time)

# ....and if there are events to process.......
if event_list != []:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if event_list != []:
if len(event_list) > 0:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced

being reviewed this week. This routine can be called multiple
times - tacking continuity loads to the start of the "master list"
# If this is a legal maneuver, process it
if pitch != 0.0:
Copy link
Member

Choose a reason for hiding this comment

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

A bit nervous about a floating-point comparison like this. Is there a way to do it differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other LR code (history-files.pl) explicitly sets the pitch to 0.0 in the NLET file if the user
did not or could not specify the 4 quaternions. The 4 Quaternion entries are set to a bogus value as well. Since we know the pitch ought not get below 45, and 0.0 is not possible then
explicitly setting will work. It is converted from a string value - not a calculation.

Copy link
Member

Choose a reason for hiding this comment

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

Can you make a comment to this effect here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly I added the explanation.

@@ -1248,7 +1155,7 @@ def Trim_bs_cmds_Before_Date(self, trim_date, cmd_list):
# Continuity text files of each load starting with the
# base load, and record the continuity information in a numpy array
#
# INPUTS: base_dir (e.g. '/data/acis/LoadReviews/2017/')
# INPUTS: base_dir (e.g. '/data/acis/LoadReviews/2017/')2021:179:04:00:02.313
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this shouldn't have been added here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@jzuhone jzuhone left a comment

Choose a reason for hiding this comment

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

So I made a few suggested changes (some of which you can actually just add as commits by hitting the button below if you agree with them).

The other thing I want to mention is that it's not clear that using astropy.time.Time and the cxcsec property is always guaranteed to be in sync with the CxoTime class, which is what we use elsewhere in the code. Tom may have a better idea about this since he's a developer for both.

I remember (I think) that one motivation for this was to try to run things outside of ska and in a regular python environment, but it seems to me that it may just be easier to install CxoTime in said environment by itself, because I don't think it has any other ska dependencies.

So for those reasons, my preference would be to use CxoTime for consistency across packages.

@Gregg140
Copy link
Contributor Author

Regarding the use of Astropy Time, the difference and rationale was discussed in detail and approved in the PR for another tool where Astropy Time was used:

acisops/lr#27

All of that rationale applies here, with the exception of the time needed to load ska. I now need to use the NLET system for applications outside of thermal models - for example in calculating recovered science time obtained from ECS measurements that are below 5000 second in exposure time: non-load events can change the actual recovered science time. This is not a ska app and should be possible to run anywhere on any machine, including home machines and MIT without requiring anything other than python3 and Astropy.

The subtle difference between Cxotime and Astropy time was mentioned in that PR. I researched the Github issues and found the issue entry regarding this difference. Regression testing shows that there is no difference in time stamps in the gold vs new states.dat and temperatures.dat files.

For lighter, more portable, faster executing code which does not require the parsing and numerous capabilities of CxoTime (only converting known formats yday <-> cxcsec) I prefer to use Astropy Time.

@jzuhone
Copy link
Member

jzuhone commented Oct 18, 2021

Regarding the use of Astropy Time, the difference and rationale was discussed in detail and approved in the PR for another tool where Astropy Time was used:
acisops/lr#27

This is not a ska app and should be possible to run anywhere on any machine, including home machines and MIT without requiring anything other than python3 and Astropy.

I guess the difference here is that this tool needs to interact directly with Ska packages (even if it could be used elsewhere), but the case in acisops/lr#27 didn't.

The subtle difference between Cxotime and Astropy time was mentioned in that PR. I researched the Github issues and found the issue entry regarding this difference. Regression testing shows that there is no difference in time stamps in the gold vs new states.dat and temperatures.dat files.

Right, I'm just worried that this won't always be true, though it may be true now. I wouldn't be surprised if cxcsec was eventually deprecated in astropy.time in favor of cxotime, since I suspect the latter will (maybe) eventually be turned into a standalone package that doesn't require installation via the Ska method.

I'm not going to make this a blocker, but it's something we need to keep in mind for the future.

@@ -1248,7 +1155,7 @@ def Trim_bs_cmds_Before_Date(self, trim_date, cmd_list):
# Continuity text files of each load starting with the
# base load, and record the continuity information in a numpy array
#
# INPUTS: base_dir (e.g. '/data/acis/LoadReviews/2017/')
# INPUTS: base_dir (e.g. '/data/acis/LoadReviews/2017/')2021:179:04:00:02.313
Copy link

Choose a reason for hiding this comment

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

Is that trailing timestamp intentional?

Copy link
Member

Choose a reason for hiding this comment

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

@Gregg140 fixed that already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already fixed

# write_back_chain_to_pickle - Given a backchain, write the contents of the
# backchain to a pickle file. If this is ACIS format
# the full path to the OFLS directory is written out.
# Attribute Sets and Gets
Copy link

Choose a reason for hiding this comment

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

Missing description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment explained in email - it's just a comment showing that the location of any set and get methods are here

The previous version of Backstop History and acis_thermal_check (ATC) placed a major loop
inside the ACISStateBuilder class of state_builder.py wherein ATC was controlling the assembly
of a backstop history. Presently, ATC detects when to end the backchaining and also assesses
the types of loads (Normal, TOO, SCSD-107 Full Stop) for each load in the backchain.
Copy link

Choose a reason for hiding this comment

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

SCSD --> SCS, and also there should be a comma after 107

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

inside the ACISStateBuilder class of state_builder.py wherein ATC was controlling the assembly
of a backstop history. Presently, ATC detects when to end the backchaining and also assesses
the types of loads (Normal, TOO, SCSD-107 Full Stop) for each load in the backchain.
These fucntions belong inside Backstop History so that All ATC does is request an assembled
Copy link

Choose a reason for hiding this comment

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

functions misspelled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

necessary command sequences (e.g. LTCTI's, Maneuvers, SCS-107 etc.) a CR*.backstop.hist
file is written out to the directory of the load under review: the root of the file name
being the same as the review load CR*.backstop file, with ".hist" appended. This .hist file
is writtern out in CR*.backstop format and can be run through a models. This provides
Copy link

Choose a reason for hiding this comment

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

model singular

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

logger message. The level of verbosity is captured by ATC and passed to the ACIS state
builder.

Also includes changes John made which ake the role of the run_start argument much more clear.
Copy link

Choose a reason for hiding this comment

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

ake --> make

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

processed and converted to a dict in the SKA.parse format. Each dict is appended
to an output list.
"""
These functions allows the user to convert a FOT, ACIS, LTCTI, RTS file, along with
Copy link

Choose a reason for hiding this comment

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

allow singular

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link

@jazan12 jazan12 left a comment

Choose a reason for hiding this comment

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

Looks good Gregg. Only small typos and maybe some missing description flagged by me. Check it out and proceed.

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