Add top-level "limits" key in xija JSON specification

[taldcroft]

Based on discussions in recent working groups, and informal support in #69, this issue proposes the formal addition of a top-level keyword limits in the xija JSON model specification format. This key would be a dict that can contain the limit keywords shown below. All keywords would be optional, except that if any values are provided then a unit keyword must be supplied.

Keywords

In general the keywords use the pattern <limit_system>.<limit_type>.<high/low>.<optional qualifier>.

ODB caution and warning limits

• odb.caution.low
• odb.caution.high
• odb.warning.low
• odb.warning.high

Planning limits

• planning.warning.low : "low planning limit" for ACIS DPA for example
• planning.warning.high : "planning limit" for most systems (do not exceed)
• planning.caution.low : no systems currently use this AFAIK
• planning.penalty.high : "penalty limit" for ACA

Unit for all values

• unit : degF or degC. Defaults to degC.

Examples:

# aca_spec.json

    "limits": {
        "aacccdpt": {
            "planning.warning.high": -5.8,
            "planning.penalty.high": -6.8
        }
    },

# dpa_spec.json
        "limits": {
            "odb.caution.high": 39.5,
            "planning.warning.high": 37.5
        },

@jzuhone @jeanconn @jskrist @matthewdahmer

Closes https://github.com/sot/ska-projects/issues/125

[taldcroft]

See also the project-level issue https://github.com/sot/ska-projects/issues/125.

[jzuhone]

@taldcroft I love this idea. In fact, this was the first thing I tried in #69. I think I just reversed the changes instead of rewriting the history, so I should be able to bring them back easily.

[jeanconn]

OK, I had missed this issue. With regard to using "planning" limits for anything not ODB, are we better off with the capability to just freely label these thins? That was my suggestion in

sot/ska-projects#125

But my thought there just speaks to the idea that the MCC API would just be plotting lines for anything not-ODB. If we have other uses for planning limits, that's a different conversation.

[jzuhone]

@taldcroft what color would you like the ACA penalty limit to be?

[taldcroft]

@taldcroft what color would you like the ACA penalty limit to be?

Chartreuse? 😄 Well, whatever is the standard "planning_caution_*" color. I don't really care as long as it is not red and is visible on the plots with a white background.

[jzuhone]

I wasn't aware that there is a "planning caution" color. I think the only limit that exists with this designation is the ACA limit.

The planning warning limits are usually plotted in green.

[taldcroft]

Indeed ACA is currently the only subsystem that uses this, but it seems like a natural generalization of the caution/warning terminology used for the ODB / glimmon limits. Maybe it would make sense to be more specific and use e.g. odb_caution_high etc?

[jzuhone]

In my working copy I currently call it "aca_penalty":

    def annotate_limits(self, ax, dir='h'):
        if len(self.limits) == 0:
            return
        draw_line = getattr(ax, 'ax{}line'.format(dir))
        if 'acisi' in self.limits:
            draw_line(self.limits['acisi'], ls='-.', color='blue')
        if 'aciss' in self.limits:
            draw_line(self.limits['aciss'], ls='-.', color='purple')
        if 'aca_penalty' in self.limits:
            draw_line(self.limits['aca_penalty'], ls='-.', color='dodgerblue')
        if 'planning_low' in self.limits:
            draw_line(self.limits['planning_low'], ls='-', color='green')
        if 'planning_high' in self.limits:
            draw_line(self.limits['planning_high'], ls='-', color='green')
        if 'caution_low' in self.limits:
            draw_line(self.limits['caution_low'], ls='-', color='gold')
        if 'caution_high' in self.limits:
            draw_line(self.limits['caution_high'], ls='-', color='gold')
        if 'warning_low' in self.limits:
            draw_line(self.limits['caution_low'], ls='-', color='red')
        if 'warning_high' in self.limits:
            draw_line(self.limits['caution_high'], ls='-', color='red')

[taldcroft]

[EDIT] - I'm not in favor of this idea.

Hmm, I wonder if we should abstract this out a bit and define a limit as a dict with name, value, unit, color keys. Then we don't need this tight coupling between code and the spec files, at least not in gui_fit. We do still need some requirement on standard values for name as MCC has specific requirements to take actions based on particular limits. Hmm.

The aca_penalty is a tricky one because that is a characteristic that is fundamentally defined in proseco.

[jeanconn]

A good question about proseco vs aca penalty limit. Probably not the place for this conversation, but what is your intuition @taldcroft on the likelihood of many aca penalty limit increases while keeping the penalty equation the same?

If it is likely, it seems like we'd want to just manage the limit with the json file so we could do a quick release to update. If we need a proseco release to change the limit, that will be a full matlab release. Or we'd need some other limit / penalty limit management strategy.

[taldcroft]

Looking back, can we just go with my original suggestion (with the odb_ prefix), but supplemented by the ACIS-specific values as needed (and used only by gui_fit)? The point is that those values can be unambiguously interpreted by other applications like MCC, independent of subsystem. At least that's the idea.

I would also suggest using more verbose/descriptive names for aciss and acisi, since at least I don't know what precisely those would refer to.

[taldcroft]

And basically the ACA penalty limit is pretty much a planning_caution_high limit, which is to say when the probability model may be less accurate and one should exercise some caution.

[taldcroft]

likelihood of many aca penalty limit increases while keeping the penalty equation the same?

With the current proseco code this will happen each time we raise the planning_warning_high limit for ACA. Sadly, this needs to be done with a characteristics code change since proseco knows only about the hardwired aca_t_ccd_penalty_limit (https://github.com/sot/proseco/blob/a676aee285909f62cb45316ecdc52c5c3e994707/proseco/characteristics.py#L34)

[jeanconn]

Specifically, I was asking about the possible value of changing proseco to take a supplied penalty limit (could be passed from ORViewer for our use case).

[matthewdahmer]

EDIT:
You can disregard my earlier comments on the colors (deleted). The colors in John's comment above look good to me. I'll note that the ACA penalty limit is shown in gray in MCC, so that could be a candidate color for the planning_caution_high/aca penalty limit instead of blue.

[taldcroft]

Specifically, I was asking about the possible value of changing proseco to take a supplied penalty limit

Sorry, ADD, didn't quite read between the lines. Yeah, that is probably a good idea.

[jeanconn]

Per TWG today, it sounds like if we want to delegate the limits to TWG as a managed thing in the model file (with the limits as conceptually a distinct thing from the model) we'll need to bring that process change to FDB too (which is fine, just wanted to capture it).

[matthewdahmer]

I'm sorry we didn't have time to focus on this in much depth today, this really deserves a separate TWG discussion.

From my initial perspective, I don't see the need to delegate thermal planning_warning limit management to the TWG, it can, and probably should, remain an FDB controlled item, as this currently fits well in the mission planning guideline management framework. The odb_warning limits are controlled at the FDB level and the odb_caution limits should remain controlled at the FOM level. Should any of these limits be changed using existing processes, the TWG would simply give approval to update the files to reflect already-approved limits.

The caution_planning limits could be controlled at the TWG, where such limits are specified (e.g. ACA penalty limit). Is this the process change that would require FDB approval?

[jeanconn]

One subtlety with regard to this is that I think of the model and the limits as being somewhat intimately connected. You can change the "effective" limits quite easily by changing the model, so at that point does it make as much sense for the limits to be independently controlled by FDB? Worth more discussion.

And of course this github issue of keeping the limits in the JSON file is a code/process item separate from the "who should need to sign off" part, but I wasn't sure where to keep track of these elements/ideas. Thanks!

[matthewdahmer]

Yes they are connected, but as they both impact safety, I think it is appropriate for one of these elements to be FDB controlled, and I'd rather the FDB control the planning_warning limit as this is more straightforward than the model.

They way I see it, the planning_warning limit is the goal, set with a reasonable expectation of the model error. Although this error can change, the goal is to keep the model error at or below that required to prevent an odb_caution violation most of the time, or to prevent unexpected performance impacts, such as with the ACA limit. This can take the form of updating the model when the observed error becomes too large, or, as a last resort, moving the planning_warning limit to account for larger error.

I too want to keep track of these elements/ideas and this seems to be a good place for the time being, though I will try to remember to link to this conversation, and summarize it, when it gets added as a TWG topic. Thank you!

[taldcroft]

What I was suggesting in the meeting was to have names like planning.caution.high.penalty or planning.caution.high.acisi. This quasi-hierarchy allows slightly more flexibility, e.g. you could write planning.caution.high.acis_i or even planning.caution.high.data_quality.acis_i or planning.caution.high.data_quality.acis_s.

Obviously this requires changes to existing spec files and code, but maybe this gets us into a good footing for future enhancements? Thoughts?

[jskrist]

I like the clean look of this quasi-hierarchy naming convention, but I'm hesitant due to the possible confusion this could lead to when reading through the code. it looks like it implies that planning is an object with caution and warning properties, which are each objects with low and high properties, etc.

This doesn't really matter to the functionality of the code, but adds a hurdle to visually parsing the code and leads me to wonder if we shouldn't make this hierarchy in the json, if it is indeed what we want, e.g.

{
    "limits":{
        "planning":{
            "caution":{
                "low":10,
                "high":100
            },
            "warning":{
                "low":5,
                "high":110
            }
        }
    }
}

[taldcroft]

I feel like that structural hierarchy just complicates the application code without providing any real benefit (but maybe I'm not seeing it?). I also think this is harder for humans to read the JSON than the flat version. E.g. by the time you get to the 110 entry, you need to scan up 7 lines to see that is a planning limit and not odb.

if 'planning.caution.low' in limits:
    # plot that line

vs.

try:
    limit = limits['planning']['caution']['low']
except KeyError:
    pass
else:
    # plot that line

There is some precedent, e.g. the logging package uses dot-separated names to infer a hierarchy of loggers, with the intent being to use the dot-separated string names of modules within a package.

[jskrist]

I agree that the "proper" hierarchy is cumbersome, and that it doesn't really add any significant benefits.

My only real argument is that using a language access syntax (i.e. the .), in a naming convention rubs against my personal programming philosophy. This (I admit) is not a strong argument.

[matthewdahmer]

I agree with Tom, adding any additional hierarchy adds complication without any real benefit. In addition to making this part of the model specification file harder to read as a human, such a hierarchy makes it less flexible. After all, using a name like "planning.caution.high" is just a convention and can coexist alongside other naming conventions. The only rules we need to abide by here are:

1. Construct a name that is easy to parse.
2. Choose a name that clearly indicates the intent of the limit (e.g. avoid names like "matts.somewhat_useful.limit").

[taldcroft]

Looking at this again after a few hours, maybe we can fit things a bit more neatly into a schema like <limit_system>.<limit_type>.<high/low>.<optional qualifier>. This would be a suggestion but not requirement. With this it would be e.g. planning.data_quality.high.acis_i and planning.penalty.high (for the ACA planning limit).

[matthewdahmer]

That seems reasonable. This adds an easy to read field that would be easy to parse and clearly indicates the intent.

[jeanconn]

Should we update the "top" of this issue with the final schema?

[taldcroft]

Should we update the "top" of this issue with the final schema?

Done.

[jzuhone]

Should this issue be closed now?