Fix Issue 1137

[iprafols]

Description of the changes:

• Added FITORDER keyword in the header FIT_METADATA from delta attributes files
• Removed FITORDER keyword in the header STACK_DELTAS from the delta attributes files
• picca_(x)cf and picca_dmat now require the delta attributes files in order to read the order variable. If not passed, the code looks for it in the standard placing
• I also updated the other scripts (picca_wick,...) that were failing tests. I'm not sure if we decided to remove the tests for these files or not.
• If the order variable cannot be read then crash the code (as opposed to continue with order=1). This is the main change in behaviour
• In the tests, the fits comparison was not complaining if the new files had new keys in the headers. This is a dangerous behaviour since it means that new things we decide to will to be tested in the future
• I updated the test suite with the changes in the headers
• I updated the correlation tests to include the delta_attribute arguments since the file does not follow the standard placing

[andreufont]

Let me try the branch at NERSC.

We also need to think about new picca_cf runs on old deltas, that do not have the header info. With the new code these would crash... This is annoying, but we might have to default to one afterall. Or add an option --order in picca_cf/xcf/dmat/xdmat so that we can recycle old delta files.

[iprafols]

We also need to think about new picca_cf runs on old deltas, that do not have the header info. With the new code these would crash... This is annoying, but we might have to default to one afterall. Or add an option --order in picca_cf/xcf/dmat/xdmat so that we can recycle old delta files.

We could write a more detailed error message with the bit of code to add the missing keyword in the file. I'd need to confirm it, but I'm pretty confident we could even recover the correct value used for that run from the delta_extraction config files that are saved with the results

[andreufont]

As we discussed on Zoom, let's try to use the .config.ini file to recover this information.

[iprafols]

I added a bit of code to enhance backwards compatibility. Pseudo code to explain the workflow:

if delta_attributes is None:
   assume standard naming for the delta_attributes file

try to read FITORDER from FIT_METADATA HDU
if file is correct but the keyword is not present (older delta_attributes file)
    1. try to read FITORDER from the STACK_DELTAS 
    if 1 does not work
        2. try to read it from the deltas config file (.config.ini)
        if 2 does not work: crash
if file cannot be opened (e.g. file not found because logs were saved in a non-standard place)
    1. try to read it from the deltas config file (.config.ini)
    if 1 does not work: crash

[andreufont]

That's great, Ignasi. Is it ready for me to try it out?

[andreufont]

@iprafols - I managed to make it work, after doing a minor change to the code:

 -        delta_attributes = in_dir + "../Log/delta_attributes.fits.gz"
 +        delta_attributes = in_dir + "/../Log/delta_attributes.fits.gz"

With this, it was able to find the value stored in the STACK (these were old deltas), and I could see a message saying "setting the order to 0".

[iprafols]

The missing / must be because you did not end the in_dir variable with it. Having two such symbols is not an issue so this is an easy fix (I already pushed the commit). In the email I received your message said something about an error? What was it? Is it fixed? Did the code work?

[andreufont]

All good, the error was only when running things on a terminal, no problem with slurm scripts

[andreufont]

@iprafols - should we worry about "true-continuum", or "raw-analysis" runs? In these analyses we should not need to look at the order variable (there is no continuum fitting involved), so I'd imagine that there is no "attributes" file associated to it. Would the code crash now? It shouldn't...

Similar for runs using LyCAN. There is no distortion, so we shouldn't need to find the order variable.

[iprafols]

I have been checking where order is used (and how). As expected, it is only used in the distortion matrix computation. Indeed, some analyses will not need it, and the code should not crash, but we still want it to crash if not given when we are computing the distortion matrix.

I thought of having order=None as a default (which I implemented), but the way this is coded, it would mean effectively choosing order=0 if the distortion matrix is computed. Essentially, the extra term is computed with an if order==1 clause. Thus, I have added a couple of checks in the computation of the distortion matrix so that the code crashes (and points to the warnings we added) if order is None. @andreufont, can you take a look in case I missed some instances?

[andreufont]

The variable "order" should be used in two places: in the computation of the distortion matrices, and in the "projection" of deltas. Neither of these should be done in true-continuum or LyCAN analyses. I agree the code should crash if order is None in these two functions.

[iprafols]

I had missed the projection, thanks! A similar issue was happening; the code was behaving as order=0 even when order=None. I fixed this and improved the docstrings

[andreufont]

I only added a couple of minor comments on docstrings, a suggested crash point, and if you have time and energy a request for a separate function def find_order to hide some of the new code from read_deltas.

[andreufont]

I confirm that the code works well both for old deltas (guessing the right location) and for new deltas.