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

Adjust earthshine effect based on Earth phase #117

Closed jzuhone closed 2 years ago

jzuhone commented 2 years ago

Description

NOTE: The PR is finished but there is no expected hurry to merge it since I anticipate it will generate some discussion.

This PR allows the heating on the ACIS FP from the earthshine into the ACIS radiator to depend on the Earth phase. The existing code assumes that the relevant earthshine is in the infrared, which should to zeroth order be the same regardless if the Earth is illuminated or not by the Sun. This code is an experimental feature to test the hypothesis that the model for the effect of earthshine on the temperature could be improved if we accounted for the Earth phase.

This PR adds this capability by using the solar ephemeris along with the spacecraft ephemeris to compute the fraction of the Earth which is illuminated by the Sun. It is currently an optional calculation through the use of kwargs. Note that this is a simple calculation which assumes that the ACIS radiator can see the whole Earth.

This calculation improves the fit of the ACIS FP thermal model at high temperatures during radzones, which was demonstrated at the TWG on 3/3/22.

Interface impacts

If the Earth phase factor is not included, the same model results are obtained as with the previous xija version.

Testing

Unit tests

Models with and without the Earth phase factor were checked to ensure they ran as expected.

taldcroft commented 2 years ago

This could also allow models to have a telemetry format dependence in the future (for fitting, most likely) if it were ever desired.

I am having trouble seeing how telemetry format could play into a thermal model, but maybe I'm missing something. There are likely some coincidental correlations with format, but not causation right?

Since this is completely unrelated it would probably be best to put 555c291 into a separate PR.

jzuhone commented 2 years ago

@taldcroft the motivation for including the telemetry format is that during FMT1 we are not getting ACIS DEA HKP data at frequent intervals and it is sometimes helpful when fitting the ACIS FP model in xija_gui_fit to examine these periods. I found it helpful in my latest fitting. You are correct that it is not causative.

I'm happy to remove it and put it in a different PR if necessary.

taldcroft commented 2 years ago

OK, now I understand about the TlmFormat. So just go ahead with a separate PR.

matthewdahmer commented 2 years ago

I tested this PR on chimchim with a local Xija installation by generating the ACIS FP dashboard plot with your new acisfp_spec.json (https://github.com/jzuhone/chandra_models/blob/fp_recal_2022/chandra_models/xija/acisfp/acisfp_spec.json). Everything seemed to work.

jzuhone commented 2 years ago

@taldcroft I will add the extra info to the PR description. Should I bundle this with the new thermal model in a single FSDS?

jzuhone commented 2 years ago

@taldcroft @matthewdahmer one note--with the added parameter k2 that I have now included, the current flight ACIS FP model no longer works with this proposed version of xija, since it does not have the required number of parameters. I suppose this is fine given that we'd want to promote both this version and the new ACISFP model in https://github.com/sot/chandra_models/pull/80, but I wanted you to be aware.

matthewdahmer commented 2 years ago

@jzuhone @taldcroft, If I remember correctly, the new ACIS FP model will require changes to the Matlab FOT Tools to pass in new parameters. We should get that sorted out before submitting the FSDS for the thermal model. The new version of Xija should be promoted first and included in the FOT MATLAB Tools version of Ska so @jskrist has the supporting Xija code available to develop against.

jzuhone commented 2 years ago

@matthewdahmer

The new version of Xija should be promoted first and included in the FOT MATLAB Tools version of Ska so @jskrist has the supporting Xija code available to develop against.

But if we promote this version of xija and install it without the new model, we cannot run the existing ACIS FP flight model, correct? It does not have the new parameter.

matthewdahmer commented 2 years ago

@jzuhone Sorry, I read what you wrote too quickly. I thought I heard you say during the TWG meeting that the current version of the ACIS FP model would work with the new version of Xija, however I see you are saying this is not the case.

We just need to:

  1. Make sure this Xija update does not make it into the FOT Matlab Tools Ska before the Matlab code is updated.
  2. Avoid merging the new ACIS FP model into the main/master branch of chandra_models before we are ready to use it for load reviews so we don't need to reverse-merge it if we have another model we find we need to promote first.

We should tag up with @jskrist during our follow-on meeting after the MPCWG to see what his schedule looks like and how he would prefer we proceed. I'll bring this up during our Matlab Dev meeting tomorrow as well.

jeanconn commented 2 years ago

I also note that doesn't seem quite clear from the interface impacts? If I understand correctly, should this PR be updated to be more backward compatible and then make the breaking change at the next version?

matthewdahmer commented 2 years ago

This PR was included in FSDS-25 and is now approved.

jeanconn commented 2 years ago

Right, so since the FSDS was brought as a github PR, I think this can get merged and a new release cut.