Adjust earthshine effect based on Earth phase #117

jzuhone · 2022-03-02T17:54:12Z

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

Mac

Functional tests

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

taldcroft · 2022-03-21T17:59:23Z

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 · 2022-03-21T18:12:51Z

@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

The code here looks basically fine, with some minor comments. The PR itself needs docs on the testing, interface impacts (e.g. making sure it doesn't change existing results for the case without solar ephemeris) etc.

Is there benefit to getting this into the upcoming Ska release?

taldcroft · 2022-03-21T18:04:01Z

xija/component/heat.py

+            solar_xyzs = [getattr(self, 'solarephem0_{}'.format(x))
+                          for x in ('x', 'y', 'z')]
+
+            if not None in solar_xyzs:


Linters will probably say this should be if None not in solar_xyzs:. I'm try to get away from nitpicky comments, but here there is real ambiguity about precedence not (None in solar_xyzs) or (not None) in solar_xyzs. It is probably the former since the code is working, but this could be worth fixing.

taldcroft · 2022-03-21T18:08:19Z

xija/component/heat.py

@@ -640,6 +654,21 @@ def dvals(self):
            else:
                self.calc_earth_vis_from_taco(ephems, q_atts)

+            # This next bit optionally checks to see if the solar ephemeris
+            # was passed in, and if it was it computes the fraction of the
+            # Earth's surface that is illuminated by the Sun. 


Include a link or reference to the TWG meeting where you presented this, so we can get back to that nice hand-drawn picture of the math.

taldcroft · 2022-03-21T18:13:15Z

xija/component/heat.py

+                cos = np.sum(ephems*solars, axis=1)
+                es = np.sum(ephems*ephems, axis=1)*np.sum(solars*solars, axis=1)
+                cos /= np.sqrt(es)
+                self._dvals *= 0.5*(1.0+cos)


Should there be a convenient way to get access to this multiplicative factor via plotting or at least as an attribute to allow for diagnostics? I don't remember enough of the details to know how hard this would be.

taldcroft · 2022-03-21T18:19:22Z

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

… not

taldcroft

I ran the unit tests on Mac with a pass. Thanks for the changes, looks good now.

matthewdahmer · 2022-03-22T13:54:51Z

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 · 2022-03-22T14:39:01Z

@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 · 2022-03-22T19:10:57Z

@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 sot/chandra_models#80, but I wanted you to be aware.

matthewdahmer · 2022-03-22T19:34:08Z

@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 · 2022-03-22T20:15:12Z

@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 · 2022-03-22T21:12:35Z

@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:

Make sure this Xija update does not make it into the FOT Matlab Tools Ska before the Matlab code is updated.
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 · 2022-03-23T13:52:15Z

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 · 2022-04-20T15:33:54Z

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

jeanconn · 2022-04-20T15:36:07Z

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

jzuhone added 2 commits February 14, 2022 16:09

Add a feature that accounts for Earth illumination

3dfcefa

Make this calculation optional

3f78f9a

taldcroft reviewed Mar 21, 2022

View reviewed changes

jzuhone mentioned this pull request Mar 21, 2022

ACIS FP model update for Spring 2022 sot/chandra_models#80

Merged

Better check for not supplying solarephem components correctly

b6b6dff

jzuhone force-pushed the solar_illum branch from 6fbc158 to b6b6dff Compare March 21, 2022 18:30

jzuhone added 5 commits March 21, 2022 14:56

Small code simplifications

5087776

Improved syntax

aac2e15

Keep the earth phase around and add a plotting option

b2a2b79

Make this two parameters--one proportional to the Earth phase and one…

9e87100

… not

Add note about TWG

1ce7f6a

taldcroft approved these changes Mar 22, 2022

View reviewed changes

Make this an optional parameter

29e34c7

jeanconn approved these changes Apr 20, 2022

View reviewed changes

taldcroft merged commit 568144b into sot:master Apr 20, 2022

This was referenced Apr 20, 2022

Update sot/xija to 4.26.0 sot/skare3#830

Closed

2022.4 sot/skare3#834

Merged

jzuhone mentioned this pull request May 17, 2022

Check and plot the ACIS FP safety planning limit, support the new ACIS FP model acisops/acis_thermal_check#50

Merged

2 tasks

javierggt mentioned this pull request May 18, 2022

2022.5 (closed) sot/skare3#850

Closed

javierggt mentioned this pull request Aug 3, 2022

2022.5 sot/skare3#899

Merged

javierggt mentioned this pull request Jan 4, 2023

Update sot/xija to 4.19.2 sot/skare3#986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust earthshine effect based on Earth phase #117

Adjust earthshine effect based on Earth phase #117

jzuhone commented Mar 2, 2022 •

edited

Loading

taldcroft commented Mar 21, 2022

jzuhone commented Mar 21, 2022

taldcroft left a comment

taldcroft Mar 21, 2022

jzuhone Mar 21, 2022

taldcroft Mar 21, 2022

jzuhone Mar 21, 2022

taldcroft Mar 21, 2022

jzuhone Mar 21, 2022

taldcroft commented Mar 21, 2022

taldcroft left a comment

matthewdahmer commented Mar 22, 2022

jzuhone commented Mar 22, 2022

jzuhone commented Mar 22, 2022

matthewdahmer commented Mar 22, 2022

jzuhone commented Mar 22, 2022 •

edited

Loading

matthewdahmer commented Mar 22, 2022

jeanconn commented Mar 23, 2022

matthewdahmer commented Apr 20, 2022

jeanconn commented Apr 20, 2022

Adjust earthshine effect based on Earth phase #117

Adjust earthshine effect based on Earth phase #117

Conversation

jzuhone commented Mar 2, 2022 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

taldcroft commented Mar 21, 2022

jzuhone commented Mar 21, 2022

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft Mar 21, 2022

Choose a reason for hiding this comment

jzuhone Mar 21, 2022

Choose a reason for hiding this comment

taldcroft Mar 21, 2022

Choose a reason for hiding this comment

jzuhone Mar 21, 2022

Choose a reason for hiding this comment

taldcroft Mar 21, 2022

Choose a reason for hiding this comment

jzuhone Mar 21, 2022

Choose a reason for hiding this comment

taldcroft commented Mar 21, 2022

taldcroft left a comment

Choose a reason for hiding this comment

matthewdahmer commented Mar 22, 2022

jzuhone commented Mar 22, 2022

jzuhone commented Mar 22, 2022

matthewdahmer commented Mar 22, 2022

jzuhone commented Mar 22, 2022 • edited Loading

matthewdahmer commented Mar 22, 2022

jeanconn commented Mar 23, 2022

matthewdahmer commented Apr 20, 2022

jeanconn commented Apr 20, 2022

jzuhone commented Mar 2, 2022 •

edited

Loading

jzuhone commented Mar 22, 2022 •

edited

Loading