-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
tighter control flow logic for calcDelays #585
Conversation
@@ -29,7 +29,14 @@ def main(): | |||
|
|||
sys.argv = [args.process, *unknowns] | |||
|
|||
from RAiDER.cli.raider import calcDelays, downloadGNSS, calcDelaysGUNW | |||
if args.process == 'calcDelays': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This control flow reduces the total import load
tools/RAiDER/cli/raider.py
Outdated
@@ -313,6 +313,10 @@ def calcDelays(iargs=None): | |||
elif len(wfiles)==1 and len(times)==2: | |||
logger.warning('Time interpolation did not succeed, defaulting to nearest available date') | |||
weather_model_file = wfiles[0] | |||
|
|||
elif (interp_method == 'center_time') and len(times)==1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the interp_time option
@@ -185,6 +185,8 @@ def set_output_xygrid(self, dst_crs=4326): | |||
out_proj = CRS.from_epsg(dst_crs.replace('EPSG:', '')) | |||
except pyproj.exceptions.CRSError: | |||
out_proj = dst_crs | |||
except AttributeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small bug that was giving me grief.
Please do not merge until we sort out control flow for GUNWs - this is more urgent. See #584 |
@cmarshak the error message mentioned in #584 is the same as what I was getting for the time-interp case. I suspect the issue is the same/similar, most likely having to do with the specific number of files returned and having an exact time. |
I agree - but unless this is tested on the exact GUNW that I mentioned in the error then this should not be merged... |
To be clear, the control flow in the main script requires concentration/thought. If we merge this and the GUNW logic still fails, then this affects the GUNW production. We can in theory always re-run the RAiDER docs to fix this issue. #584 will be fixed EOD tomorrow. |
I cannot necessarily recreate the error from the cloud if this is merged first. |
02179d7
to
3d950dd
Compare
I had a headache writing #593 in part because of the try/except handling here in I am concerned as an outsider that generally, if To elaborate on the latter point, if I am user of Raider and I specify interpolation as Additionally, having these try/except without failure leads to confusing errors like those in #587 (or at least confusing to me). The error doesn't indicate that input data was not available (which in the GUNW case, should have led to an error or exit much sooner). Also, if the control flow is being added, I would encourage the use pytest-mock (or something similar) to make sure whatever logic you want (and errors) is happening as you expect without doing any actual delay calculations. Maybe a unit test could help me understand the problem with the CLI that you and Brett need, too. |
Hey @cmarshak, sorry about your headaches! This control flow logic isn't the cleanest, and I've updated this PR to try to at least make the code more readable. You have a good point about the failures. I usually think from a scientific user point of view - I want a weather model to use, and I'd like to use time-interpolation, but if it doesn't happen I really don't care, just get me a weather model. I've used many codes that post lots of errors because of various detailed problems when I just want a quick-and-dirty result I can work with. For the GUNW workflow that's obviously not what we want, but for general RAiDER use my inclination is to give the user something to work with and raise warnings about what they got. For this PR, I've made it explicit for azimuth time interpolation that three files are required or the program will fail. |
Hey Jeremy - do whatever works for the multiple uses of RAiDER - no need for my opinions to block that. I hope that general thoughts can provide helpful pointers to a slightly different viewpoint. Any large code bases will have headaches. I think the ISCE2 way of thinking is to put everything into a log and carefully look at the logs after if there are failures. I personally think it's easier from a debugging point of view to have errors that are causing any time of failures to be captured at the end of standard i/o. Thanks for merging that monolith of a PR - I will test these new updates this morning and post on their respective issue tickets if they are success. Also, maybe something that wasn't clear that definitely should be flagged is not the azimuth time interpolation uses 2 or 3 times. 2 times are used when the acquisition time occurs between two model times with a 5 minute buffer. Otherwise 3 times are used. That means simply looking at the number of times retrieved alone could lead to more unexpected behaviors. Finally, paraphrasing from
|
Description
Motivation and Context
How Has This Been Tested?
Type of change
Checklist: