Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Spitzer logic #362

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Fix Spitzer logic #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e26477
Fix #358
troyraen Dec 8, 2024
7706496
Apply @jkrick review comments
troyraen Dec 10, 2024
441fbac
Bugfix: Return a Table instead of a Row
troyraen Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions spectroscopy/code_src/mast_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
def JWST_get_spec(sample_table, search_radius_arcsec, datadir, verbose,
delete_downloaded_data=True):
"""
Retrieve HST spectra for a list of sources and groups/stacks them.
Retrieve JWST spectra for a list of sources and groups/stacks them.
This main function runs two sub-functions:
- `JWST_get_spec_helper()` which searches, downloads, retrieves the spectra.
- `JWST_group_spectra()` which groups and stacks the spectra.
Expand All @@ -31,7 +31,7 @@ def JWST_get_spec(sample_table, search_radius_arcsec, datadir, verbose,
Search radius in arcseconds.
datadir : str
Data directory where to store the data. Each function will create a
separate data directory (for example "[datadir]/HST/" for HST data).
separate data directory (for example "[datadir]/JWST/" for JWST data).
verbose : bool
Verbosity level. Set to True for extra talking.
delete_downloaded_data : bool, optional
Expand Down Expand Up @@ -60,7 +60,7 @@ def JWST_get_spec(sample_table, search_radius_arcsec, datadir, verbose,
def JWST_get_spec_helper(sample_table, search_radius_arcsec, datadir, verbose,
delete_downloaded_data=True):
"""
Retrieve HST spectra for a list of sources.
Retrieve JWST spectra for a list of sources.

Parameters
----------
Expand All @@ -70,7 +70,7 @@ def JWST_get_spec_helper(sample_table, search_radius_arcsec, datadir, verbose,
Search radius in arcseconds.
datadir : str
Data directory where to store the data. Each function will create a
separate data directory (for example "[datadir]/HST/" for HST data).
separate data directory (for example "[datadir]/JWST/" for JWST data).
verbose : bool
Verbosity level. Set to True for extra talking.
delete_downloaded_data : bool, optional
Expand Down
10 changes: 5 additions & 5 deletions spectroscopy/code_src/spitzer_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

def SpitzerIRS_get_spec(sample_table, search_radius_arcsec, COMBINESPEC):
"""
Retrieve HST spectra for a list of sources.
Retrieve Spitzer spectra for a list of sources.

Parameters
----------
Expand Down Expand Up @@ -47,14 +47,14 @@ def SpitzerIRS_get_spec(sample_table, search_radius_arcsec, COMBINESPEC):

# If multiple entries are found, pick the closest.
# Or should we take the average instead??
if len(tab) > 0:
if len(tab) > 1:
print("More than 1 entry found", end="")
if not COMBINESPEC:
print(" - pick the closest")
Comment on lines 48 to 53
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

sep = [search_coords.separation(SkyCoord(tt["ra"], tt["dec"], unit=u.deg, frame='icrs')).to(
u.arcsecond).value for tt in tab]
id_min = np.where(sep == np.nanmin(sep))[0]
tab_final = tab[id_min]
id_min = np.nanargmin(sep)
tab_final = tab[[id_min]] # double brackets to return a Table instead of a Row
else:
print(" - Combine spectra")
tab_final = tab.copy()
Expand All @@ -63,7 +63,7 @@ def SpitzerIRS_get_spec(sample_table, search_radius_arcsec, COMBINESPEC):

# Now extract spectra and put all in one array
specs = []
for tt in tab:
for tt in tab_final:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

url = "https://irsa.ipac.caltech.edu{}".format(tt["xtable"].split("\"")[1])
spec = Table.read(url, format="ipac") # flux_density in Jy
specs.append(spec)
Expand Down