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

Fix ECS timing, silence Matplotlib warnings, code cleanup #11

Merged
merged 9 commits into from
Jul 20, 2022

Conversation

jzuhone
Copy link
Member

@jzuhone jzuhone commented Jul 20, 2022

Description

This PR addresses the following issues.

  • The ACIS long ECS commanding no longer waits an extra 10012 seconds past the end of the ECS run to enable RADMON, so we remove that time from the SimulateECSRun calculation.
  • In DatePlot and CustomDatePlot, no longer use the fmt keyword to avoid warnings from Ska.Matplotlib such as these:
/data/acis/miniconda3/envs/ska/lib/python3.8/site-packages/Ska/Matplotlib/core.py: UserWarning: linestyle is redundantly defined by the 'linestyle' keyword argument and the fmt string "-b" (-> linestyle='-'). The keyword argument will take precedence.
 ax.plot(dates, y, *args, **kwargs)
/data/acis/miniconda3/envs/ska/lib/python3.8/site-packages/Ska/Matplotlib/core.py: UserWarning: color is redundantly defined by the 'color' keyword argument and the fmt string "-b" (-> color='b'). The keyword argument will take precedence.
 ax.plot(dates, y, *args, **kwargs)
  • Remove unnecessary subclassing of object
  • Add a new set_yscale method to DatePlot and CustomDatePlot
  • Use static model JSON files in the thermal model testing suite to get consistent answers

Interface impacts

None except the above.

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Functional tests

A number of ACIS-related cron jobs which run various tasks depend on ACISpy for a range of functionality, these have run without issue on a test install of this PR. More importantly, the Matplotlib warnings went away.

@jzuhone jzuhone requested review from Gregg140 and jazan12 July 20, 2022 17:42
Copy link

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

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

Looks fine

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.

Hi John,

This looks fine. The only minor thing I wanted to double-check is I thought we were back to -88 as the perigee FP planning limit until matlab tools were updated. (Probably the model update already went in and I just don't remember, but in any case want to confirm. Assuming that's the case, all good by me.)

@jzuhone
Copy link
Member Author

jzuhone commented Jul 20, 2022

Hi @jazan12, the model file update here is just a JSON file for testing, so strictly speaking we don't need the limit to be -88 C here, especially since it is going back. But thanks for pointing that out.

@jzuhone jzuhone merged commit 4f4eb8f into acisops:master Jul 20, 2022
This was referenced Oct 5, 2022
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.

None yet

3 participants