-
-
Notifications
You must be signed in to change notification settings - Fork 420
Bugfix: ALMA enhanced tables don't need qtable #3395
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
base: main
Are you sure you want to change the base?
Conversation
Wow, how does this not raise an error, is it not covered by tests? |
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.
Just guessing around what the problems might be
@@ -220,7 +220,7 @@ def get_enhanced_table(result): | |||
"Please refer to https://astropy-regions.readthedocs.io/en/latest/installation.html for how to install it.") | |||
raise | |||
|
|||
def _parse_stcs_string(input): | |||
def s_region_parser(input): |
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.
I would leave this as a private method
return prep_table | ||
|
||
for row in result: | ||
row['s_region'] = s_region_parser(row['s_region']) |
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.
don't reuse/call the method inside its body
@@ -293,19 +293,10 @@ def _get_region(tokens): | |||
return result | |||
else: | |||
return _get_region(s_region.split()) | |||
prep_table = result.to_qtable() |
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.
I'm not at all sure why this stopped working, we haven't really switch the API (as we haven't really switched out the TAP results to return Tables rather than DAL result tables as suggested #3244)
This example results in an error:
error:
This PR fixes the problem by skipping the
to_qtable
conversion.However, the test is failing:
so I've not clearly solved the problem.
@andamian can you chime in?