tighter control flow logic for calcDelays

[jlmaurer]

• some products fail due to incorrect flow control in the main raider.py CLI

Description

• Instead of running the command, the CLI errors out at the weather model / date checking stage
• Custom exceptions allow for more nuanced handling of errors
• Control flow is more readable by parsing out specific tasks to their own functions
• azimuth time interpolation only works if all three files can be found, center_time will work with at least one file

Motivation and Context

• with three options now for time interpolation (or not), we need to have better-defined flow control for processing

How Has This Been Tested?

• Raider-docs runs now
• some additional unit tests written and pass (more currently in the works)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• I have added an explanation of what your changes do and why you'd like us to include them.
• I have written new tests for your core changes, as applicable.
• I have successfully ran tests with your changes locally.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[jlmaurer]

This control flow reduces the total import load

[jlmaurer]

This fixes the interp_time option

[jlmaurer]

This is a small bug that was giving me grief.

[cmarshak]

Please do not merge until we sort out control flow for GUNWs - this is more urgent. See #584

[jlmaurer]

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. get_nearest_wmtimes is the relevant function.

[cmarshak]

I agree - but unless this is tested on the exact GUNW that I mentioned in the error then this should not be merged...

[cmarshak]

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.

[cmarshak]

I cannot necessarily recreate the error from the cloud if this is merged first.

[cmarshak]

I had a headache writing #593 in part because of the try/except handling here in calcDelays in raider.py.

I am concerned as an outsider that generally, if prepareWeatherModel doesn't complete, shouldn't that be an error as there are numerous reasons that this function couldn't finish whether it be a download issue (transient) or something intrinsic to RAiDER that someone hasn't considered in the weather model preparation? Or if not, is there a more explicit exception that could be raised for what you are expecting (i.e. a custom RAiDER exception that explains what's expected)? The code would become a lot simpler too allowing the code to error out IMHO. Not to mention, there could be potentially unintended consequences.

To elaborate on the latter point, if I am user of Raider and I specify interpolation as center_time (I believe none requires the nearest time relative to requested time to be retrieved), then I can get different results for the same inputs if prepareWeatherModel has transient issues e.g downloading a product. Is that desired or common? Is there another common error related to the notebooks you ran? Why is it OK for expected weather model times to be ignored when one fails downloading? This is concerning as a novice when the model step times are 3 hours or 6 hours, in which you probably(?) do not just want to get the datetimes that were available, but the ones desired. Is there another error that you also are considering?

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.

[jlmaurer]

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. center_time will still try to work under certain cases with a warning. I've defined several custom exceptions to be more explicit about types of failures, and RAiDER will only continue without failure in appropriate cases. Testing all this will take a bit of time though, so hopefully there is enough time to get it done right.

[cmarshak]

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 import this:

Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
Now is better than never.
Although never is often better than *right* now.