adding xrt_teem

[jslavin]

Adding xrt_teem.py only

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[namurphy]

Thank you again for doing this! I'm excited to have this added! I only took a quick look so far. I'll try to look at this again in the next day or two.

I noticed that the .fits files were giving pre-commit some indigestion, so I made two quick pull requests to tell some of the pre-commit hooks to ignore .fits files and update the pre-commit config. Assuming you have upstream set as the remote for the XRTpy repo, doing git pull upstream main and git push should get the changes merged into this branch.

With regards to documentation, a lot of what's in XRTpy is based off of PlasmaPy, so the PlasmaPy doc guide might be helpful here too.

[joyvelasquez]

pre-commit.ci autofix

[joyvelasquez]

@jslavin If possible, add documentation to the section of Getting Started - XRTpy Objects for users to get more information about xrt_teem before the squash and merge. However, this can be a later pull request of its own as well.

[wtbarnes]

I've not taken a close look at this, but one high level suggestion would be to make the two input XRT images sunpy map objects rather than accepting two data header pairs. This would vastly simplify the metadata handling as SunPy already includes code for handling XRT headers. For example, the parsing of the filter wheel combination is already done on the XRT map source in SunPy and can be accessed as a property.

I would also suggest that the return type of the emission and temperature also be SunPy maps. That way, a user can continue to take advantage of all the usual operations (e.g. cropping, visualization) on their derived temperature maps.

[wtbarnes]

Mark wants us to maintain fairly close correspondence of the XRTpy code with the IDL code.

Understood

By the way, I ran into some odd bugs at first when trying to make plots from the map outputs. For some reason, getting the date attribute to a map produced errors when I included the 'timesys' keyword in the metadata. In XRT data that has the value 'UTC (TBR)'. I found that there was no problem if I included the entire metadata from the image.

I suspect the issue is because, in the XRT map source, there is a fix for the non-standard TIMESYS entry "UTC (TBR)"
https://github.com/sunpy/sunpy/blob/a846ce09322cd6ded64da35ab0dc3ec90d970b44/sunpy/map/sources/hinode.py#L63-L67. When you include all of the metadata from the original image, Map recognizes this as an XRT map and thus handles the key appropriately. However, when you only use subset of the metadata, specifically when you don't set the INSTRUME key to "XRT", Map does not know how to handle this TIMESYS key since the fix is only applied for XRT maps. When you remove the TIMESYS key, map falls back to the default which is UTC https://github.com/sunpy/sunpy/blob/main/sunpy/map/mapbase.py#L815

I agree with you re: not including all the original metadata as it no longer applies to the resulting EM/temperature maps. My suggested fix would be to set the TIMESYS keyword to "UTC". Note also that if you use the header helper function (see my comment above), this will be handled for you.

I have now implemented returning Maps from xrt_teem. However I identified one behavior that's different between the IDL routine and the XRTpy routine. Using the superpixel method for a masked image, the summing is done treating the masked pixels as 0s. The mask itself is not summed, so the net effect is that any summed pixel will not be masked unless all the pixels that make it up are masked. The IDL routine on the other hand makes any binned pixel masked if it has any of its pixels masked. One could argue either way about the proper way to do that, though certainly you're not getting a properly summed pixel if some of the original pixels are masked. To change what is returned by superpixel would require modification of the code.

That's an interesting point. In sunpy, the logic of masked pixels in superpixel is handled by NumPy masked arrays which set masked pixels to zero when summing. Would you mind creating an issue about this on the sunpy repo? I would be interested to hear other's opinions on this behavior and whether there is room for flexibility.

Note that you could you could recreate the kind of mask you're talking about by passing the mask attached to your original Map to reshape_image_to_4d_superpixel (this is the function used by the superpixel method) and then summing over the appropriate dimensions.

[namurphy]

Thank you again for doing this! I especially liked the writing style for the docstrings and example notebooks, since I found it quite understandable. A lot of my comments are related to reStructuredText formatting and thus should be fairly straightforward to implement.

My philosophy with code review is for the reviewer to offer thoughts and suggestions, and then usually have the code author decide whether or not to accept the suggestion since they're the expert on it. So, if there are suggestions that you disagree with or would prefer to address in a different way, please take the approach that you decide would be best.

Thank you again!

[joyvelasquez]

pre-commit.ci autofix

[namurphy]

When we add a new module, we often need to add a stub documentation file so that the docstrings from the module show up in the online documentation.

[namurphy]

I forgot to add this error code as one of the linter checks to ignore. This one checks for undefined substitutions, but isn't smart enough to check for our globally defined substitutions.

[namurphy]

@jslavin — I made a few edits directly to this pull request to try getting the docstring to show up in the documentation preview for xrt_teem, and to ignore a linter error regarding substitutions. It'll be necessary to do a git pull to get the updates.

There seems to be a problem with the reStructuredText references to other packages not forming links. This may be an issue with how intersphinx is set up, though I wasn't able to find any problems. I'll have to look into this a bit more to figure out what's happening here. I think there was also just a new release of Sphinx, which might(?) have something to do with it. In any case, that's outside the scope of this pull request, so it shouldn't hold this up.

[jslavin]

Hi Nick, The documentation looks good. Thanks for your work in getting that done. Jon

…

On Mon, Feb 13, 2023 at 6:36 PM Nick Murphy ***@***.***> wrote: @jslavin <https://github.com/jslavin> — I made a few edits directly to this pull request to try getting the docstring to show up in the documentation preview for xrt_teem <https://xrtpy--89.org.readthedocs.build/en/89/api/xrtpy.response.xrt_teem.xrt_teem.html#xrt-teem>, and to ignore a linter error regarding substitutions. It'll be necessary to do a git pull to get the updates. There seems to be a problem with the reStructuredText references to other packages not forming links. This may be an issue with how intersphinx is set up, though I wasn't able to find any problems. I'll have to look into this a bit more to figure out what's happening here. I think there was also just a new release of Sphinx, which might(?) have something to do with it. In any case, that's outside the scope of this pull request, so it shouldn't hold this up. — Reply to this email directly, view it on GitHub <#89 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE5OY3EXUIHHCIHSD53H5LWXLAPHANCNFSM6AAAAAASO4RUVQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>