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 fewer dP pitches + add sensible keyword arg defaults #107

Closed taldcroft closed 3 years ago

taldcroft commented 3 years ago

Description

The long-term trending of SolarHeat power likely has a smoother dependence on pitch than the power itself, and fitting these parameters is often not well constrained. This is sometimes seen as unphysical variations when fitting the dP (power trending) parameters (effectively overfitting). This PR allows having fewer pitch bins for the dP parameters.

I suspect that having 4 to 6 bins (e.g. [45, 90, 135, 180]) is good enough and reduces the model dimensionality.

In the course of trying to write a test for this which covers all of the different SolarHeat subclasses, I ran into a problem that the different subclasses required different positional arguments for various telemetry components. I realized that these telemetry component arguments like pitch_comp or simz_comp could sensibly have defaults set, e.g. 'pitch' and 'sim_z'. This works and in practice is always the same as what gets set in current model specs.

The functional part to allow fewer dP pitch bins is in 55aa8a5, while the argument munging and some other tidying is in f996b81.

Testing

Functional testing

TODO: ensure the full ACIS regression test suite passes.

jzuhone commented 3 years ago

Really like this!

matthewdahmer commented 3 years ago

I generated a functional test for this change that passes, based on the current ACA model spec. To run this test I created two model files:

  1. I created a version of the ACA model where I modified the JSON model spec to set all dP parameter values to 0.04, but kept the same number of pitch values, and made no other changes to this file.
  2. I then created a modified version of this JSON file that used only two dP pitch values, both of which were set to 0.04. This required edits to both the "comps" section and the "pars" section.

I ran each of these using the fewer-dP-pitches Xija branch, and found they produced identical results.

taldcroft commented 3 years ago

Thanks @matthewdahmer, very helpful!

@jzuhone - would you be able to run the full regression tests for one of the ACIS checkers?

jzuhone commented 3 years ago

@taldcroft yes, I will try that this week.

taldcroft commented 3 years ago

@jzuhone - ping on the ACIS regression tests for this.

jzuhone commented 3 years ago

Argh, yes. Setting a reminder to do it this week.

jzuhone commented 3 years ago

@taldcroft all tests pass, so you should be good to go.

taldcroft commented 3 years ago

Thanks @jzuhone !