Add default and set to zero masking option for specreduce operations.

[cshanahan1]

(incomplete draft, just look at tracing and background for now)

[codecov]

Codecov Report

Attention: Patch coverage is 37.20930% with 54 lines in your changes missing coverage. Please review.

Project coverage is 45.56%. Comparing base (6b8e995) to head (d4d528d).

Files with missing lines	Patch %	Lines
specreduce/core.py	18.91%	30 Missing ⚠️
specreduce/tracing.py	58.33%	15 Missing ⚠️
specreduce/background.py	22.22%	7 Missing ⚠️
specreduce/extract.py	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #216       +/-   ##
===========================================
- Coverage   83.37%   45.56%   -37.81%     
===========================================
  Files          13       13               
  Lines        1137     1196       +59     
===========================================
- Hits          948      545      -403     
- Misses        189      651      +462     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tepickering]

finally got a chance to at least look through the code changes. i like it so far! let me know when it's ready for a fuller review and i'll try playing with it in more depth.

[camipacifici]

I am pretty sure it is! Please go ahead. Thank you!

[tepickering]

there are a bunch of warnings in the RTD build. they look like malformed references, but need a deeper look to see what's the actual cause.

[tepickering]

i'm confused why python 3.8 and 3.9 tests are being run. a rebase might be needed, though i thought this was forked after those tests were removed.

[tepickering]

i think this is one of the RTD complaints. should be ~specreduce.tracing.Trace, i believe.

[tepickering]

should be ``FlatTrace`` or a proper sphinx reference

[tepickering]

here and elsewhere need to use double single-quotes for inline literal/code. pretty sure this is what RTD has been complaining about...

[hpparvi]

_get_data_from_image is a static method, so self is not defined. This could be fixed by making _mask_and_nonfinite_data_handling a static method as well and calling it as _ImageParser._mask_and_nonfinite_data_handling(...), but this would require removing the self.image.mask reference in line 144.

Also, _ImageParser never assigns an image to self.image. This could be fixed by changing the line 55

img = self._get_data_from_image(image, disp_axis=disp_axis)
return img

into

self.image = self._get_data_from_image(image, disp_axis=disp_axis)
return self.image

Then again, _ImageParser seems to be meant to be a pure mixin class that provides the image parsing functionality but doesn't store information. The original version of _parse_image in #144 does assume that the object has an image attribute that is returned if no image is given

if image is None:
    # useful for Background's instance methods
    return self.image 

but I think this is quite dangerous since the code assumes that the class inheriting from _ImageParser sets the image attribute by default and that it does this before _ImageParser._parse_image(...) is called.

[cshanahan1]

This has become kind of a mess and i'm trying to figure out how to reorganize. A while back I had to move _get_data_from_image to a static method because some of the 'Background' methods that create Background objects call it before the object is instantiated. I am trying to work off the existing code and avoid refactoring in this PR, but I may have to. This option I'm exploring right now is changing _ImageParser from a mixin class to either just a regular class or set of functions that is called within each step, and the arguments are passed in each time.

[hpparvi]

The only way to avoid refactoring _ImageParser that I can see would be to turn _mask_and_nonfinite_data_handling into a static method as well and to provide mask_treatment as a passed argument (and to handle checking whether the mask treatment method is valid in the calling code.) This way, the _get_data_from_image class method could be used the way it is now.

However, there wouldn't be much advantage to having the image parsing functionality as a class instead of a function, and I think refactoring _ImageParser into a function might make the most sense. Turning it into a regular class would also work, but I'm not sure what would be again the advantage of this over having the image parsing as a function if we want to be able to parse an image inside a class method (as with the Background methods).

Then again, we don't necessarily need to do the image parsing in the Background.two_sided and Background.one_sided class methods since it is done in Background.__post_init__ by default (unless I'm missing something). So, another option would be to leave the image parsing inside Background.__post_init__ and remove it from the two class methods, in which case the Background object would always be instantiated before parsing the image.

If you want to avoid major refactors in this PR, I could open a separate PR where I could refactor the _ImageParser into a function, and we can see how much sense this approach would make. What do you think?

[hpparvi]

This method definition overrides the previous _get_data_from_image definition that handles masking.