Skip to content
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

Heasarc: trying votable #2353

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

volodymyrss
Copy link
Contributor

@volodymyrss volodymyrss commented Mar 31, 2022

in response to #2333 and #2349, this is an attempt to use VOTable backend of heasarc to resolve problems with unit conversion.
It needs to be determined if this is a viable approach.

@@ -252,7 +252,7 @@ def _parse_result(self, response, verbose=False):

try:
data = BytesIO(response.content)
return Table_read(data, hdu=1)
return Table_read(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to get rid of the Table_read workaround, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, yes.
Though I think it will be still needed for the fallback, old interface has no VOTable.

I copied the code from #2333 to a test:

https://github.com/oda-hub/astroquery/blob/b09c8b588bafa0078c2a6996b251af669a1a894d/astroquery/heasarc/tests/test_heasarc_remote.py#L108-L123

Currently it fails while trying to _convert_record2fits. Maybe if it starts with fits it's more compatible. Some investigation is needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to get rid of the Table_read workaround, too.

I have been trying to work on this conversion to VOTable on my own. Is there any other reason why we need to keep the Table_read function in other than silence the unit parsing?

For refernce:

def Table_read(*args, **kwargs):
    if commons.ASTROPY_LT_5_1:
        return Table.read(*args, **kwargs)
    else:
        return Table.read(*args, **kwargs, unit_parse_strict='silent')

I only ask since I in some testing with some of my own changes I've cut out this function and most tables work with VOTable perfectly fine without warnings.

I can submit a separate PR with more info if that would be more appropriate.

@bsipocz bsipocz added this to the v0.4.7 milestone Mar 31, 2022
@bsipocz bsipocz changed the title trying votable Heasarc: trying votable Mar 31, 2022
@pep8speaks
Copy link

Hello @volodymyrss! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 110:9: E265 block comment should start with '# '
Line 113:36: E225 missing whitespace around operator
Line 114:35: W291 trailing whitespace
Line 117:9: E265 block comment should start with '# '
Line 117:9: E303 too many blank lines (2)
Line 119:27: E261 at least two spaces before inline comment
Line 120:96: E251 unexpected spaces around keyword / parameter equals
Line 120:98: E251 unexpected spaces around keyword / parameter equals
Line 120:111: E251 unexpected spaces around keyword / parameter equals
Line 120:113: E251 unexpected spaces around keyword / parameter equals
Line 122:9: E265 block comment should start with '# '
Line 122:60: W291 trailing whitespace
Line 123:58: E251 unexpected spaces around keyword / parameter equals
Line 123:60: E251 unexpected spaces around keyword / parameter equals

@nkphysics nkphysics mentioned this pull request Oct 16, 2023
@bsipocz bsipocz removed this from the v0.4.7 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants