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

BUG: fix parsing by filtering out junk lines #2719

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Apr 28, 2023

This closes #2717

I'm on the fence whether this needs a changelog or not, I suppose it does as lines from the docs stopped working due to these junk lines, e.g. this one

>>> response['FREQ'].quantity

@bsipocz bsipocz added this to the v0.4.7 milestone Apr 28, 2023
@bsipocz bsipocz requested a review from mkelley April 28, 2023 01:57
@bsipocz
Copy link
Member Author

bsipocz commented Apr 28, 2023

This also fixes all the doctesting failures we currently see in the docs (some are due to these lines, but some are due to upstream changes)

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #2719 (66f2099) into main (1ba336e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2719   +/-   ##
=======================================
  Coverage   65.74%   65.74%           
=======================================
  Files         233      233           
  Lines       17844    17844           
=======================================
  Hits        11732    11732           
  Misses       6112     6112           
Impacted Files Coverage Δ
astroquery/jplspec/core.py 91.78% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

except for minor typo, looks great

@mkelley
Copy link
Contributor

mkelley commented Apr 28, 2023

This looks good to me, except that the LGINT units comparison failed for me locally:

Package versions: 
Numpy: 1.21.4
Scipy: 1.7.1
Matplotlib: 3.5.2
h5py: 3.4.0
Pandas: 1.3.3
Astropy: 5.0.1
regions: not available
pyVO: not available
mocpy: not available
astropy-healpix: 0.6
vamdclib: not available
astropy-helpers: 4.0.1

Using Astropy options: remote_data: any.

rootdir: /home/msk/Projects/astroquery, configfile: setup.cfg
plugins: cov-2.12.1, typeguard-2.13.3, xdist-2.5.0, arraydiff-0.3, requests-mock-1.9.3, remotedata-0.3.2, hypothesis-6.21.6, anyio-3.6.1, forked-1.4.0, mock-3.6.1, openfiles-0.5.0, filter-subpackage-0.1.1, astropy-header-0.1.2, asdf-2.8.1, doctestplus-0.11.0
collected 1 item                                                                                                                                                                         

docs/jplspec/jplspec.rst F                                                                                                                                                         [100%]

======================================================================================== FAILURES ========================================================================================
_________________________________________________________________________________ [doctest] jplspec.rst __________________________________________________________________________________
027 .. doctest-remote-data::
028 
029    >>> from astroquery.jplspec import JPLSpec
030    >>> import astropy.units as u
031    >>> response = JPLSpec.query_lines(min_frequency=100 * u.GHz,
032    ...                                max_frequency=1000 * u.GHz,
033    ...                                min_strength=-500,
034    ...                                molecule="28001 CO",
035    ...                                get_query_payload=False)
036    >>> print(response)
Differences (unified diff with -expected +actual):
    @@ -1,4 +1,4 @@
    -    FREQ     ERR    LGINT   DR   ELO   GUP  TAG   QNFMT QN' QN"
    -    MHz      MHz   nm2 MHz      1 / cm
    +    FREQ     ERR    LGINT   DR   ELO    GUP  TAG   QNFMT QN' QN"
    +    MHz      MHz   MHz nm2      1 / cm                          
     ----------- ------ ------- --- -------- --- ------ ----- --- ---
     115271.2018 0.0005 -5.0105   2      0.0   3 -28001   101   1   0

@keflavich
Copy link
Contributor

I think the lgint comparison is related to astropy version - possibly this PR:
https://github.com/astropy/astropy/issues?q=order++units

@bsipocz
Copy link
Member Author

bsipocz commented Apr 28, 2023

Oh, great. Then I need to double check whether I use the same version as ci.

@bsipocz
Copy link
Member Author

bsipocz commented Apr 28, 2023

OK, so this is the PR that brake stuff, and the suggested workaround for a global pin in setup.cfg won't do it for us:
astropy/astropy#14439

We do run remote-data tests for the latest astropy release (5.2.x currently), and with the dev version, so I would vote for making these changes for the dev test and live with the breaks for the windows online tests until 5.3 comes out rather than adding hacks.

This change affects the jplspec module, and cdms, and svo_fps, and only the narrative documentation. Fix for the new formatting is in this PR and in #2721.

@bsipocz
Copy link
Member Author

bsipocz commented Apr 28, 2023

Side note: I'm very much looking forward to use scientific-python/pytest-doctestplus#197, so our rst doctesting won't be bogged down at a doctest failure for a print command.

@bsipocz bsipocz merged commit 6fe15b7 into astropy:main Apr 28, 2023
9 checks passed
@bsipocz bsipocz deleted the jplspec_fix_CADDIR_parsing branch December 8, 2023 18:25
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.

BUG: JPLSpec: parsing looks off
3 participants