Enable missing background flag in GOL2AF for local background subtraction

[duytnguyendtn]

Two weeks of spelunking to get 4 diffs, such is the cost of debugging in C; new developers take heed lest ye fall as did I!

The way each of the python wrapper subroutines were written was to check if each argument existed in the catch all **params argument and if so, chain them into the command list. In GOL2AF, back was set as an explicit, named argument in the function definition. As such, back would never have been found in the **params dictionary. I opted to remove this named argument from the function declaration such that the back argument would therefore be added to the **params dictionary.

I'll also add a modified version of @st-apidgeon's testing notebook demonstrating the local background subtraction code running

[rosteen]

Good catch! That's a rather subtle bug.

[bjkuhn]

Hi all, sorry for the delay with testing the local background subtraction, but I finally have some notes to share. First, I provide a list of the steps I followed before running the test:

• Downloaded hstaXe using development version instructions on the github page • Downloaded Jupyter notebooks and data from Box folder • Edited my local copy of axelowlev.py and axetasks.py to match the code changes in Duy’s (this) PR • Went through IR Jupyter notebook to test local background subtraction

Next, I provide a list of my comments:

1. I’m getting the same results for no background subtraction and local background subtraction. This could be an operator error but I’m pretty sure I’m running things correctly.
2. When I try to turn on local background subtraction in the axecore step I cannot set interp = 0. It crashes with aXeError: AXECORE: The parameter 'interp' must be set for the background PETs! The traceback points to line 325 of axetask.py and line 292 of inputchecks.py. In the past, interp = 0 worked and should correspond to an average interpolation. I can get axecore to run if I set interp to something other than 0 e.g. -1 (median interpolation).
3. When I’m able to get axecore to run (interp != 0), it’s taking between 1.5 - 2.5 hours to complete! This is not normal and I’m wondering if anyone might know why? The only thing I know of that is different is the installation method; this is my first time running the notebook from the development version, although I’m not certain if that would cause slowdowns or not.

Does anyone have any questions or suggestions for me to try? Thanks!

[duytnguyendtn]

Hi @bjkuhn, sorry to hear that! Out of curiosity, which notebook did you try to run? Does the IR background subtraction demo notebook I prepared run successfully in your environment? https://stsci.app.box.com/file/990351817636

[bjkuhn]

Hi @duytnguyendtn, I ran both your version of the notebook and Aidan's. Both would not allow interp = 0 and both were giving me the same results for local background subtraction on and off.

[st-apidgeon]

Sorry I'm a bit late to contribute here, but I experienced Ben's issues 1 and 2 in my tests as well. axecore is taking a normal amount of time to run for me, though.

Re: issue 1, I was using the notebook from @duytnguyendtn, and saw that the log indicated the subtraction had been performed when running the notebook as it came, but when comparing pixel values from the STP files with those from files with all bacground keys switched off, I got the exact same results. Ben and I are going to tag up to ensure we've been doing things the same way, so we'll keep you posted.

As far as Ben's issue number 2 goes, I think this is because on line 289 ofinputchecks.py, in check_axecore, we've got if back and not interp:, so having interp = 0 is causing a problem.

I'm going to keep messing around to see if I can get some other method to work. Thanks for bearing with us on this!

[duytnguyendtn]

Thanks to @rosteen's #37, the docs are much easier to read now and I think points us in the right direction! I can confirm that I saw the same thing @st-apidgeon found: the code currently requires interp!=0. As @bjkuhn suggests here and is confirmed by the docs, 0 is a valid interpolation mode meant to represent "average". I created #38 to fix that bug

As for the output, this is the point where I'll need to lean on the science staff for more guidance as to where to go from here. This review suggests there may need to be a thorough review of the algorithm itself, which will be a much larger effort than this PR's focus, that was to "Figure out why it wasn't running and turn it on." To help with the review process, I think we should split these two efforts into:

1. Why back=True is crashing and preventing the local background subtraction from running (sounds like is fixed by this PR)
2. Fix the average interpolation bug that @bjkuhn and @st-apidgeon found (#38)
3. Why isn't local background subtraction resulting in the results we're expecting (a new ticket)

How does this sound everyone?

[rosteen]

3. Why isn't local background subtraction resulting in the results we're expecting (a new ticket)

Personally, I think that it's worse to enable the flag for local background subtraction when we know it isn't working properly, and thus would advocate for doing the work to get it producing the expected results as part of this PR.

[duytnguyendtn]

The reason why I'm advocating for breaking the effort is highlighted in the title for this PR: Enable missing background flag in GOL2AF for local background subtraction

The full effort is to get local background subtraction producing the correct results, I agree. But this PR specifically doesn't promise to do that, rather fix this one bug concerning the missing background flag at hand, which may be one amongst many which we don't see yet to get to our full effort's goal. I would really like to avoid this PR becoming the "Fix everything about local background subtraction" PR because we haven't investigated the recent findings of @bjkuhn and @st-apidgeon to know how large of a rabbit hole that is, since we haven't been able to run the local background subtraction code at all until now.

After these review comments, this would be a situation I could see, in an ideal case, creating a feature branch for local background subtraction. That way we could merge this PR independently and continue to investigate the results afterwards, only merging to main when we get the results we'd like. But I think we're "flying by the seat of our pants" a little bit more here 😉

[rosteen]

The reason why I'm advocating for breaking the effort is highlighted in the title for this PR

You can change the title of the PR - perhaps "Enable local background subtraction" would be sufficient.

we haven't investigated the recent findings of @bjkuhn and @st-apidgeon to to know how large of a rabbit hole that is

All the more reason not to merge this without getting the functionality it calls working. I really don't want to merge this and then have it be a month (or more!) before the local background subtraction actually works properly, given that the install instructions in the repo highlight installing from main as a recommended workflow to get hstaxe installed. Having a user try to use local background subtraction and hit an error is far better than generating an output without the code complaining and having the user not know that it didn't do what they expected.

[rosteen]

We could also add an error message that gets raised if this flag is set until we're certain the results are correct, at which point we could remove the error message (credit to @kecnry for the idea offline), but I still advocate for just getting it working here.