-
Notifications
You must be signed in to change notification settings - Fork 35
Web Clean up #662
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
Web Clean up #662
Conversation
if exact_ck_times and len(times) > 1: | ||
function_args = {"startEt": start_et+time_bias, "stopEt": stop_et+time_bias, "toFrame": d, "refFrame": s, "mission": mission, "searchKernels": self.search_kernels, "ckQualities": ["smithed", "reconstructed"], "fullKernelPath": False} | ||
futures.append(executor.submit(spiceql_call, "getExactTargetOrientations", function_args, self.use_web)) |
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 code cant really run on none-web spice. Some kernel sets will need more than one CK, causing this to fail since kernels are furnished in the __enter__
call. Considering that GetExactTargetOrientations
only causes a speed up in the web condition, I think it's fine limiting to web.
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.
Right, but you want to catch failures for all cases rather than just web cases. While the function doesn't provide a significant speed up for offline cases, I was able to find issues with the function by running the full set of tests in ALE with the updated logic.
While there are endpoints that only really benefit instances where web==True
, we need to be able to test them in an offline context so that we know it will work for all drivers rather than a select few that we test locally by setting web==True
.
if exact_ck_times and len(ephemeris_times) > 1 and not nadir: | ||
try: | ||
sensor_times = spiceql_call("extractExactCkTimes", {"observStart": ephemeris_times[0] + inst_time_bias, | ||
"observEnd": ephemeris_times[-1] + inst_time_bias, | ||
"targetFrame": sensor_frame, | ||
"mission": mission, | ||
"searchKernels": frame_chain.search_kernels}, | ||
use_web=frame_chain.use_web) | ||
times = spiceql_call("extractExactCkTimes", {"observStart": ephemeris_times[0] + inst_time_bias, | ||
"observEnd": ephemeris_times[-1] + inst_time_bias, | ||
"targetFrame": sensor_frame, | ||
"mission": mission, | ||
"searchKernels": frame_chain.search_kernels}, | ||
use_web=frame_chain.use_web) | ||
|
||
if len(times) == 0: | ||
exact_ck_times = False | ||
|
||
except Exception as e: | ||
exact_ck_times = False |
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.
Similar to my other comment, Im not sure we want to spend time with the extra call if web==True.
There might be a concern that if exact_times==False and web==True, and the sensor is a linescanner, things will take forever.
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.
Sure, the extra call here is to handle if there are multiple cks that are loaded for the image. If the function fails, then we can't get exact ck times so we set exact_ck_times
to false.
This then goes to the rotation generation that will run getTargetOrientations
rather than getExactTargetOrientations
. So you're right it may take a while for large line scanners. I think we need to address other ways of reducing those times rather than adding if else
blocks that all over ALE to handle specific cases.
Cleans up some of the web integration in ALE. These changes depend on this PR in SpiceQL
Licensing
This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words: