updated for version 9 of instrument files - [closed] #197

Closed nabobalis closed 1 year ago

In GitLab by @adwilson on Jul 15, 2020, 08:04

_Merges v9instrument -> master

This fixes failures to find epoch in JSOC correction tables by updating the instrument file urls to version 9.

It works fine as far as I can tell for any data within SDO mission life but I am not sure if any changes needed elsewhere in the code (or if my naive substition of the urls was appropriate).

In GitLab by @markcheung on Jul 15, 2020, 08:11

Hi @adwilson,

Thanks for reporting the issue and the merge request. We just noticed this yesterday afternoon. We are working with JSOC to find a way to fetch the calibration table for all versions, and not just the most recent one. This will be important for reproducibility of results. Once we have a solution to do that, we'll update the master.

So we'll wait a little before approving the merge.

Thanks, Mark

In GitLab by @adwilson on Jul 15, 2020, 08:15

Hi,

Great, no worries.

Alasdair

In GitLab by @wtbarnes on Jul 15, 2020, 09:22

I agree that I think we should make a few more changes than just those implemented here, but I'm surprised that these tests are still failing...I would think that the changes here would be sufficient to at least not make things blow up anymore.

In GitLab by @adwilson on Jul 15, 2020, 10:40

The only thing I can think is that if the test script (which I can't find the source code for sorry) does not fetch the correction table via get_correction_table() and instead uses a saved local file that is now out of date then this might produce this error. I can't think of any reasons why else this wouldn't work off the top of my head (but I am unfamiliar with the package really)

Because running the test manually locally which fails on gitlab:

In [19]: local_table=get_correction_table()
In [20]: _select_epoch_from_table(94*u.angstrom, Time('2015-01-01 00:00:00'), correction_table=local_table)                                                                                                   
Out[20]: 
<QTable length=2>
   ORIGIN    TELESCOP         DATE                 T_START                  T_STOP         VER_NUM WAVE_STR WAVELNTH WAVEUNIT   EPERDN       DNPERPHT      EFF_AREA  EFF_WVLN EFFA_P1  EFFA_P2 EFFA_P3
                                                                                                            Angstrom                                         cm2     Angstrom                         
   str12       str7          str20                  object                  object          int64    str9   float64    str8    float64       float64       float64   float64  float64  float64 float64
------------ -------- -------------------- ----------------------- ----------------------- ------- -------- -------- -------- --------- ------------------ -------- --------- -------- ------- -------
SDO/JSOC-SDP  SDO/AIA 2020-07-06T21:54:25Z 2010-03-24T00:00:00.000 2011-01-27T15:00:00.000       9  94_THIN     94.0 angstrom 18.299999 1.9767099999999997 0.296404 93.900002 0.000655     0.0     0.0
SDO/JSOC-SDP  SDO/AIA 2020-07-06T21:54:28Z 2013-10-01T12:00:00.000 2016-05-01T12:00:00.000       9  94_THIN     94.0 angstrom 18.299999 1.9767099999999997 0.262518 93.900002 -4.9e-05     0.0     0.0

In GitLab by @wtbarnes on Jul 15, 2020, 11:47

We test both inputs. See https://gitlab.com/LMSAL_HUB/aia_hub/aiapy/-/blob/master/aiapy/calibrate/tests/test_util.py#L19

I think I know why. The local ones are actually the ones failing because those only contain the v8 calibration, but now we filter on the v9 calibration which yields an empty table.

In GitLab by @adwilson on Jul 15, 2020, 12:35

added 1 commit

2b352bdc - default to latest version if no entries match

Compare with previous version

In GitLab by @adwilson on Jul 15, 2020, 12:48

added 1 commit

d533164d - added check for if any entry matches ver num

Compare with previous version

In GitLab by @wtbarnes on Jul 15, 2020, 13:08

@adwilson I've just opened up !74 which is very similar to this. Sorry, I didn't mean to have overlapping efforts on this! If you have time, would you mind looking over and or trying out that branch and seeing if it solves your problem?

In GitLab by @adwilson on Jul 15, 2020, 13:11

added 2 commits

6e9150e1 - added check for wavelength not present in table
6e2eca60 - clarified wavelength error

Compare with previous version

In GitLab by @adwilson on Jul 15, 2020, 14:01

That had been what I figured the problem was. I will try your branch tomorrow, but I am sure it solves mine (and most other) problems, even just the trivial editing of the version number I did initially fixed any issues I had.

I had just committed some changes to this branch before you added your merge which in essence took the declared version number from channel.py and when _select_epoch_from_table was called if this version was not present in the correction table it instead fell back to the maximum version number that was present in the table.

This works for the direct tests of _select_epoch_from_table since for the local file (the one with only version 8) it can fall back to newest version present (which is v8). While I quite liked that approach as it means historic correction tables will just work without having to specify a version, it does fail some tests and who can be arsed to find out why, I suspect it is due to using newer correction versions than these tests expect.

FAILED ../../.tox/py38-online/lib/python3.8/site-packages/aiapy/calibrate/tests/test_prep.py::test_degradation[None-time_correction_truth0]
FAILED ../../.tox/py38-online/lib/python3.8/site-packages/aiapy/response/tests/test_channel.py::test_eve_correction[None-None-eve_correction_truth0]

Since !74 passes those tests and seems all-over more rigorously executed then I guess it should be the way to go!

In GitLab by @wtbarnes on Jul 15, 2020, 20:34

Ah OK. Yes, I see what you're saying about not having to specify the version number if you're passing in a filename. I think though, if someone is not using the most up to date calibration (i.e. the default, hard-coded one) we should probably be failing to let them know that that is not the most up to date one, while at the same time of course allowing the user to specify their preferred version (I'll add an explicit error message for that now). As you point out, there are perfectly valid reasons why someone may want to use an older version of the calibration (e.g. reproducing an older result)!

!74 also makes the change of separating the version numbers in the channel response files and the calibration. That was my initial mistake in making them the same, but I do not actually think they are updated in tandem and so should be separate.

If it's alright with you, I'm going to go ahead and close this as it is a duplicate of !74. If there's anything that you think is missing in that merge request, please do put a comment there.

In GitLab by @adwilson on Jul 16, 2020, 01:53

Yeah please go for it, you've even included the specific error for invalid wavelength so I think there is now nothing this merge does that !74 does not cover.

Cheers,

In GitLab by @adwilson on Jul 16, 2020, 01:53

closed

LM-SAL / aiapy

updated for version 9 of instrument files - [closed] #197