#835 - Improvements to trending.plot_wfs_obs_delta & wfe_histogram_pl… #836
Conversation
…e_histogram_plot to support full filename paths
|
Hello @kulpster85, Thank you for updating !
Comment last updated at 2024-05-03 21:40:26 UTC |
webbpsf/trending.py
Outdated
| @@ -1462,17 +1464,18 @@ def plot_wfs_obs_delta(fn1, fn2, vmax_fraction=1.0): | |||
| """ | |||
| from skimage.registration import phase_cross_correlation | |||
|
|
|||
| _ = webbpsf.mast_wss.mast_retrieve_opd(fn1) | |||
| _ = webbpsf.mast_wss.mast_retrieve_opd(fn2) | |||
| if download_opds == True: | |||
There was a problem hiding this comment.
Minor: the == True here is unnecessary.
webbpsf/trending.py
Outdated
|
|
||
| opd, hdul1 = webbpsf.trending._read_opd(fn1) | ||
| opd, hdul1 = webbpsf.trending._read_opd(fn1, auto_download=download_opds) |
There was a problem hiding this comment.
I don't quite follow the logic of the change here. If download_opds is True, then the previous lines will have just downloaded those files, in which case there is no need to pass auto_download=True to the _read_opd function, because the files are already downloaded? Am I misunderstanding?
There was a problem hiding this comment.
So the reason for this is because the read_opd takes a filename but this change will allow for the full file path to be provided. So I wanted to prevent the download since the full_file_path won't exist when combined with the webbpsf_data_path.
def _read_opd(filename, auto_download=True): full_file_path = os.path.join(webbpsf.utils.get_webbpsf_data_path(), 'MAST_JWST_WSS_OPDs', filename) if not os.path.exists(full_file_path) and auto_download:
There was a problem hiding this comment.
I understand what you're trying to do but I don't see that change in _read_opd. Is that a change you intend to add to this PR? Right now the code in _read_opd is always going to append some directories on the front of the filename, regardless of the auto_download flag. I'm looking right at that function in this PR and the existing code is as follows:
def _read_opd(filename, auto_download=True):
"""Trivial utilty function to read OPD from a WSS-output FITS file
If the file does not exist locally, try to retrieve it from MAST automatically."""
full_file_path = os.path.join(webbpsf.utils.get_webbpsf_data_path(), 'MAST_JWST_WSS_OPDs', filename)
if not os.path.exists(full_file_path) and auto_download:
webbpsf.mast_wss.mast_retrieve_opd(filename)
opdhdu = fits.open(full_file_path)
opd = opdhdu[1].data.copy()
return opd, opdhdu
There was a problem hiding this comment.
it's always going to assign full_file_path and then try to open that... regardless of the download. yes?
There was a problem hiding this comment.
Yes. I removed the auto_download call in the plot_wfs_obs_delta. Turns out the os.path.join has the smarts to ignore all previous segments when joining with an absolute path segment.
os.path.join(path, *paths)
Join one or more path segments intelligently. The return value is the concatenation of path and all members of *paths, with exactly one directory separator following each non-empty part, except the last. That is, the result will only end in a separator if the last part is either empty or ends in a separator. If a segment is an absolute path (which on Windows requires both a drive and a root), then all previous segments are ignored and joining continues from the absolute path segment.
…e_histogram_plot to support full filename paths - remove unneccessary == True check
…e_histogram_plot to support full filename paths
#835 - Improvements to trending.plot_wfs_obs_delta & wfe_histogram_plot to support full filename paths