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

Two new functions to handle limit strings, minor viz fixes for xija_gui_fit #112

Merged
merged 13 commits into from
Aug 25, 2021

Conversation

jzuhone
Copy link
Collaborator

@jzuhone jzuhone commented Jul 20, 2021

Description

Based on the new limit scheme implemented in sot/chandra_models#75, this PR implements two new functions in xija, get_limit_spec and get_limit_color.

get_limit_spec takes a limit of the form <system>.<type>.<direction>.<qualifier> and returns a dict with these names as the keys:

from xija.limits import get_limit_spec
limit_spec = get_limit_spec("planning.data_quality.high.acisi")
print(limit_spec)

which gives

{
    'system': 'planning', 
    'type': 'data_quality', 
    'direction': 'high', 
    'qualifier': 'acisi'
}

This may have other uses eventually, but it was useful for get_limit_color, which uses it to assign colors for the various limits.

get_limit_color is used by xija_gui_fit to assign limit line colors, but I'll be using and get_limit_spec and get_limit_color in acis_thermal_check and ACISpy.

Functional testing consisted of running xija_gui_fit for every model in chandra_models with limits and making sure that the limit lines appeared correctly in the data__time, resid__data, and histogram plots.

This PR also fixes a number of minor visualization issues with xija_gui_fit.

Testing

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

Fixes #

@jzuhone
Copy link
Collaborator Author

jzuhone commented Aug 2, 2021

@taldcroft Wanted to see if you had any comments on this.

xija/limits.py Outdated Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
if limit == "unit":
continue
lines.append(
draw_line(limits[limit], color=get_limit_color(limit))
Copy link
Member

Choose a reason for hiding this comment

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

I think this should pay attention to the limit units (indeed many of them are not degC).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@taldcroft what do you want it to do exactly? This loop is to draw the lines and since the "unit" item is in the same dict as the limit definitions, it needs to skip over it which is the reason for lines 170-171.

Copy link
Member

Choose a reason for hiding this comment

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

E.g the limits for the PFTANK2T model are in degF, but the models are always computed and calculated in degC. Maybe I'm missing the code, but somewhere along the way the limit values need to be converted degC.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Aug 21, 2021

This is ready for review again.

xija/limits.py Outdated Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
if limit == "unit":
continue
lines.append(
draw_line(limits[limit], color=get_limit_color(limit))
Copy link
Member

Choose a reason for hiding this comment

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

E.g the limits for the PFTANK2T model are in degF, but the models are always computed and calculated in degC. Maybe I'm missing the code, but somewhere along the way the limit values need to be converted degC.

xija/gui_fit/plots.py Outdated Show resolved Hide resolved
xija/limits.py Show resolved Hide resolved
xija/gui_fit/plots.py Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
xija/limits.py Outdated Show resolved Hide resolved
Copy link
Member

@taldcroft taldcroft 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 now!

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.

2 participants