sot / xija

Thermal modeling framework for Chandra X-ray Observatory
https://sot.github.io/xija
BSD 3-Clause "New" or "Revised" License
9 stars 5 forks source link

Allow the caller to set their own DPA state power coefficients #45

Closed jzuhone closed 7 years ago

jzuhone commented 7 years ago

This PR enables the model specification to set the DPA state power coefficients by hand instead of having them hard-coded into xija. In a given model's JSON file, the argument would look like this:

{
    "class_name": "AcisDpaStatePower",
    "init_args": [
        "1deamzt"
    ],
    "init_kwargs": {
        "pow_states": [
            "0xxx",
            "1xxx",
            "2xxx",
            "3xx0",
            "3xx1",
            "4xxx",
            "5xxx",
            "55x0",
            "66x0",
            "6611",
            "6xxx"
        ],
        "ccd_count": "ccd_count",
        "clocking": "clocking",
        "fep_count": "fep_count",
        "vid_board": "vid_board"
    },
    "name": "dpa_power"
},
taldcroft commented 7 years ago

:+1: to this idea.

If this pull request is accepted, all of the thermal models which use the ACIS DPA state power as an input (presumably only the ACIS ones) would have to be updated.

It looks like you have written this to be back-compatible, so would updating actually be a requirement? Or just something that you want to do in order to take advantage of this and make the models better?

jzuhone commented 7 years ago

Sheepishly, it actually hadn't occurred to me that this actually is backwards-compatible. I just checked and it is. So I retract my earlier statement about having to change all the model specs.

taldcroft commented 7 years ago

Probably a good idea to rebase on master now (with #44) merged and test.

jzuhone commented 7 years ago

Yup, still looks good after rebase.

jzuhone commented 7 years ago

@taldcroft should this be version 3.7.2 or 3.8? I suspect the former since it's not a big addition.

taldcroft commented 7 years ago

@jzuhone - it should be 3.8 as this is a new feature and not a bug-fix. We are trying to use this convention these days. Admittedly this can lead to versions like 3.42, but so be it.

taldcroft commented 7 years ago

@jzuhone - the code looks good to me. It would be good to exercise the new functionality in unit tests. You could add a JSON model spec file that chops out one of the power states (perhaps based on the existing dpa.json?).

Also, to release and install to Flight this will need to go through the load review board for approval. Since it is back-compatible this can be any time, but you should drive that process.

jeanconn commented 7 years ago

This was approved at load review on 1-Aug-2017. I'll plan to merge and install when products are officially approved.