Astropy commentary cards not handled by munch

[vandalt]

When reading a fits file that has _HeaderCommentaryCards keys, munch seems to interpret them as mappings and looks for a .keys. This causes amical.extract_bs to crash with the error message below.

Click to see the error message

AttributeError                            Traceback (most recent call last)
<ipython-input-1-a4288c748d4a> in <module>
     99 # Extract raw complex observables for the target and the calibrator:
    100 # It's the core of the pipeline (amical/mf_pipeline/bispect.py)
--> 101 bs_t = amical.extract_bs(cube_t, file_t, targetname=target, **params_ami, display=True)
    102
    103 # %%

~/astro/AMICAL/amical/mf_pipeline/bispect.py in extract_bs(cube, filename, maskname, filtname, t
argetname, instrum, bs_multi_tri, peakmethod, hole_diam, cutoff, fw_splodge, naive_err, n_wl, n_
blocks, theta_detector, scaling_uv, i_wl, unbias_v2, compute_cp_cov, expert_plot, verbose, displ
ay)
   1155         cprint("\nDone (exec time: %d min %2.1f s)." %
   1156                (m, t - m * 60), color="magenta")
-> 1157     return dict2class(obs_result)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in munchify(x, factory)
    440         return partial
    441
--> 442     return munchify_cycles(x)
    443
    444

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in munchify_cycles(obj)
    412         seen[id(obj)] = partial = pre_munchify(obj)
    413         # Then finish munchifying lists and dicts inside obj (reusing munchified obj if
cycles are encountered)
--> 414         return post_munchify(partial, obj)
    415
    416     def pre_munchify(obj):

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in post_munchify(partial,
 obj)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in update(self, *args, **
kwargs)
    232         be defined in subclasses.
    233         """
--> 234         for k, v in iteritems(dict(*args, **kwargs)):
    235             self[k] = v
    236

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in <genexpr>(.0)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in munchify_cycles(obj)
    412         seen[id(obj)] = partial = pre_munchify(obj)
    413         # Then finish munchifying lists and dicts inside obj (reusing munchified obj if
cycles are encountered)
--> 414         return post_munchify(partial, obj)
    415
    416     def pre_munchify(obj):

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in post_munchify(partial,
 obj)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in update(self, *args, **
kwargs)
    232         be defined in subclasses.
    233         """
--> 234         for k, v in iteritems(dict(*args, **kwargs)):
    235             self[k] = v
    236

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in <genexpr>(.0)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in munchify_cycles(obj)
    412         seen[id(obj)] = partial = pre_munchify(obj)
    413         # Then finish munchifying lists and dicts inside obj (reusing munchified obj if
cycles are encountered)
--> 414         return post_munchify(partial, obj)
    415
    416     def pre_munchify(obj):

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in post_munchify(partial,
 obj)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in update(self, *args, **
kwargs)
    232         be defined in subclasses.
    233         """
--> 234         for k, v in iteritems(dict(*args, **kwargs)):
    235             self[k] = v
    236

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in <genexpr>(.0)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in munchify_cycles(obj)
    412         seen[id(obj)] = partial = pre_munchify(obj)
    413         # Then finish munchifying lists and dicts inside obj (reusing munchified obj if
cycles are encountered)
--> 414         return post_munchify(partial, obj)
    415
    416     def pre_munchify(obj):

~/.pyenv/versions/amical/lib/python3.7/site-packages/munch/__init__.py in post_munchify(partial,
 obj)
    431         # might be involved in a cycle
    432         if isinstance(obj, Mapping):
--> 433             partial.update((k, munchify_cycles(obj[k])) for k in iterkeys(obj))
    434         elif isinstance(obj, list):
    435             partial.extend(munchify_cycles(item) for item in obj)

~/.pyenv/versions/amical/lib/python3.7/site-packages/six.py in iterkeys(d, **kw)
    597 if PY3:
    598     def iterkeys(d, **kw):
--> 599         return iter(d.keys(**kw))
    600
    601     def itervalues(d, **kw):

AttributeError: '_HeaderCommentaryCards' object has no attribute 'keys'

Maybe this is an issue with how the _HeaderCommentaryCards class is defined in astropy (see here) ? The solution could be to file an issue upstream with astropy to see if this could be handled there. Otherwise, this could be handled by AMICAL if you are OK with simply removing the COMMENT and HISTORY keys when creating the oifits files. I implemented the latter fix locally and it works fine. There might also be a better solution. I'm not an expert with astropy fits handling or with munch.

Let me know what you think @DrSoulain. I can then either send a PR or fill an issue on the astropy github (or both, if you want to use the PR as a temporary fix).

[neutrinoceros]

Hi @vandalt
you did a great job at digging into this ! I think the real issue is indeed in Astropy, since they declare _CardAccessor (which _HeaderCommentaryCards derives from) as a subtype of Mapping, but fail to define a keys attribute, which is very likely a bug.
It also seems pretty easy to fix, but Astropy maintainers have a lot of experience and might find reasons why a trivial patch won't do. I suggest reporting upstream and maybe file a patch if there is no obvious obstacle. If all goes well, we can then discuss how to approach this in AMICAL (decide if we want to require the latest Astropy version or ship a workaround for currently existing versions). Thanks

[vandalt]

Hi @neutrinoceros,
I agree, this seems to cause the problem. I filed an issue on the Astropy repo (astropy/astropy#11866). In the meantime, should I send a PR that removes the _HeaderCommentaryCards before converting (they are not used anywhere in AMICAL)? I could mark it as "HACK" or "CAVEAT", but it would at least make it possible to use AMICAL with mirage simulations out of the box (another solution is that users remove the keywords before passing the fits file to AMICAL).

[neutrinoceros]

As long as there is a long-term issue opened here to keep track of it, and allow us to easily get rid of the hack once it stops being necessary, I think it's the right thing to do indeed.
Can you add a failing test first and then fix it ?

[vandalt]

@neutrinoceros The fix I have right now is implemented directly in extract_bs and simply removes the commentary cards from the header at the beginning of the function. However I just noticed that in _add_infos_header, there is an if statement that adds the header to infos only if SPHERE is not the instrument (link to code). If I remove the condition I get the same error with SPHERE as with NIRISS/mirage. I suspect it was added for this reason. Can you or @DrSoulain confirm this ? Everything seems to run fine with SPHERE if I put my fix in _add_infos_header. If I'm not missing anything here, this would allow AMICAL to preserve most of the header with SPHERE data, and it would make the fix more localized (it would only change the header when it is added in a dict, not as soon as it's open).

If you confirm this is OK, I can change it and still mark it as "HACK" with some comments. Otherwise I would still put the fix in _add_infos_header (even if it does not replace the SPHERE if statement), this would make it both clearer and easier to test.

[DrSoulain]

Indeed @vandalt, the if statement was a work around for SPHERE, with the idea to fix it properly latter. My idea, for now, is to check if the header of the instrument (no matter which it is) is compliant with munch. If I remember correctly, the problem with _HeaderCommentaryCards appeared during the "munchify" formatting (using the alias dict2class in the code). So the Astropy issue is another problem and we need to fix it. So, for me, you can just implement your solution, compatible with both SPHERE and NIRISS, and it's ok.

[vandalt]

Sounds good. Indeed the issue appears in munchify. munch checks if the object is a mapping and iterates its keys if it is. The astropy issue is directly related to this I think: astropy's _CardAccessor (parent of _HeaderCommentaryCards) is registered as a mapping, but has no keys() method, so when munch tries to access it it causes an AttributeError. This could be fixed in astropy by either not registering the class as a mapping or by adding the missing Mapping attributes. On the munch side, using hasattr(obj, "keys") instead of isinstance would fix the issue as well, but I'm not sure if it would break other use-cases on their side, it might be worth to open an issue there to ask if there are no changes in astropy.

I'll send a PR with the fix I currently have.

[vandalt]

Now that #45 is merged, I will keep an eye on the upstream issue with Astropy. Regarding my last comment, do you think it is worth opening an issue with munch as well ?

In the meantime, as @neutrinoceros mentioned in #45, this issue should probably stay open to track things upstream and remove the hack currently in use when it becomes possible.

[neutrinoceros]

I don't think there's anything wrong with munch itself, unless I've missed something ?

[vandalt]

Not really, but adding a check with hasattr or try/except (in addition to isinstance) could make it more robust to cases like this, given that it's possible to register a mapping object that has no keys.

[neutrinoceros]

So I dug up the conversation on Astropy that you started @vandalt:
looks like the fix is set to be released in Astropy 5.0, but who knows when that happens.

[DrSoulain]

This issue is still open? We have to wait for Astropy team right @neutrinoceros?

[neutrinoceros]

Actually Astropy 4.0.6 was released 9 days ago and may have the fix. I'll try reversing the temp fix now

[vandalt]

As I mentioned in #59, the Astropy change that fixes this is not included in the latest release. It is in the main branch and has been included in the Astropy 5.0 milestone, as @neutrinoceros pointed out. But I confirm that the upstream changes remove the need for the hack currently used in AMICAL (removing commentary cards before giving the header to munch).