Fix Spitzer logic

[troyraen]

Closes #358

Includes two code changes, explained below.

[troyraen]

This code change:

• appears to be what the original author intended. (print("More than 1 entry found")
• does not change the results but does make the code more efficient. (The code block that finds the closest entry now gets skipped when len(tab) == 1.)

[jkrick]

It makes sense to me that this is a typo and not intentional. I agree with the change.

Along the same lines, do we need the 'else' statement for this 'if' statment? since we have already covered the case where it is 0, it should never get to that else. Which may mean that we can get rid of the whole 'if len(tab)> 1 ...else' and reduce the indentation??? Somehow I can't leave this comment on the appropriate line, sorry.

[troyraen]

Along the same lines, do we need the 'else' statement for this 'if' statement?

That code block wasn't getting hit originally but it will now when len(tab) == 1. And/but I agree that this could be made simpler (== easier to follow).

[troyraen]

This code change does change the results, and appears to be what the author intended. The code block above this produces tab_final which contains the single row from tab that is closest to the target if COMBINESPEC is True, else a full copy of tab. In the original code, tab_final was not ever used, so COMBINESPEC had no effect -- spectra were always combined.

[jkrick]

yes, I agree that the original intent of the code is to use COMBINESPEC to determine if the spectra should be combined or chosen based on the nearest, and I agree that is not happening (good catch!). I agree this change you suggest makes sense.

[jkrick]

Thanks @troyraen for catching those logic issues. I think your suggested changes are sensible and I left comments on them in response to your comments.

I don't know why I can't leave comments on other places in the code, maybe because I wasn't tagged to review?

Some additional suggestions for the code in addition to the inline comments:

• line 14: change 'HST' to 'Spitzer' - which makes me think that the HST function should be checked for the same logic problems being fixed in this PR (Hey! I read the doctstring)
• could simplify the np.nanmin line to be np.nanargmin which returns the index directly - not important at all, but I had to look that one up and found the simpler solution

@afaisst if you have time this week, could you check this logic stuff, but if not, I am pretty confident that the changes are what the code was intended to do.

[jkrick]

It makes sense to me that this is a typo and not intentional. I agree with the change.

Along the same lines, do we need the 'else' statement for this 'if' statment? since we have already covered the case where it is 0, it should never get to that else. Which may mean that we can get rid of the whole 'if len(tab)> 1 ...else' and reduce the indentation??? Somehow I can't leave this comment on the appropriate line, sorry.

[jkrick]

yes, I agree that the original intent of the code is to use COMBINESPEC to determine if the spectra should be combined or chosen based on the nearest, and I agree that is not happening (good catch!). I agree this change you suggest makes sense.

[troyraen]

I don't know why I can't leave comments on other places in the code

You may be running into github's limit on the number of lines above or below an actual change that's being made in the PR.

[troyraen]

Thanks @jkrick. I'll look closer at the two HST and np.nanmin things.

[troyraen]

I applied your suggestions @jkrick. Good call on np.nanargmin. The HST logic is different but there were similar problems in the docstrings (probably from me copy/pasting in #356).

I think this is ready but I'll wait a bit to see if @afaisst wants to review.

[troyraen]

Rebased on top of main to pull in recent changes.

[troyraen]

Changes approved by @afaisst. Merging now.