search

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

Call a model from another model and use it as a node #125

Closed jzuhone closed 1 year ago

jzuhone commented 2 years ago

Description

This PR provides a new class that calls a separate xija model from another model and returns the temperature data as a node, which then can be used as any other component.

Still need to provide a docstring and some tests, this is just here to start discussion and review.

Interface impacts

Testing

Unit tests

Independent check of unit tests by [REVIEWER NAME]

Functional tests

No functional testing.

taldcroft commented 2 years ago

Awesome! One thing that might be good is a version parameter to specify the version of the model. I suppose most of the time you want to the latest release (which is the default) but for testing it would be good to be able to pin it.

I've been scratching my head for a few minutes and writing down, then deleting ideas, about how to make this fit in more easily with the existing structure that MATLAB and other Python tools use for initializing models. Basically there is no precedent for nested/embedded components, so I'm a bit worried about this requiring substantial upstream changes to use.

Maybe this model itself is OK and all that needs work is the HRC model to use this new model in a way that is consistent with existing APIs. So perhaps this PR needs a companion demonstration HRC model that "just works" in xija_gui_fit. At first glance that would mean that ACIS components like ccd_count or clocking would be treated as formal parameters of the HRC model but would get passed through to the embedded DPA model. Maybe the HRC model comp could be a property that does this delgation for the right components? Just thinking out loud.

I'm also curious if this serializes to JSON and Python properly.

jzuhone commented 2 years ago

@taldcroft Here's a working example in xija_gui_fit for the HRC model Dan passed to me:

image

Note the presence of the 1DPAMZT model data. Histogram:

image

Here is the JSON file you need for this:

https://gist.github.com/5119ad44f11c058dfab3399d099a3dcd

taldcroft commented 2 years ago

@jzuhone - unfortunately I'm no longer able to run xija_gui_fit, see #126.

jzuhone commented 2 years ago

@taldcroft I have a fix for this incoming.

taldcroft commented 2 years ago

Hum, just noticed it works fine for an old DPA model, so maybe it is particular to this spec file?

jzuhone commented 2 years ago

I rebased this on top of the merged #127

jzuhone commented 1 year ago

Closing this because we decided not to use it.