API: Fix splatalogue module with upstream changes by keflavich · Pull Request #2960 · astropy/astroquery

keflavich · 2024-02-27T21:56:17Z

This is a major refactor that will fix splatalogue once I've found all the corners.

Pushing now so I have a WIP saved here.

EDIT: Fixes #2956

pep8speaks · 2024-02-27T21:56:23Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-04-09 17:24:19 UTC

keflavich · 2024-03-16T15:22:01Z

This works locally. I was even able to reproduce the error that is showing up now about the wrong content type in the mocks, and fix it, but the same error is recurring now. I can't make sense of it now. The content keyword in the mock is very clearly a b'' that has a decode attribute.

keflavich · 2024-03-16T15:24:34Z

ach, nevermind my previous comment; I didn't realize where the mocks were defined (they're in conftest)

codecov · 2024-03-16T15:30:15Z

Codecov Report

Attention: Patch coverage is 81.31868% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 66.81%. Comparing base (886817e) to head (e4b3bbd).
Report is 5 commits behind head on main.

❗ Current head e4b3bbd differs from pull request most recent head 9f2c437. Consider uploading reports for the commit 9f2c437 to get more accurate results

Files	Patch %	Lines
astroquery/splatalogue/core.py	78.94%	16 Missing ⚠️
astroquery/splatalogue/setup_package.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2960      +/-   ##
==========================================
- Coverage   66.83%   66.81%   -0.02%     
==========================================
  Files         237      236       -1     
  Lines       18327    18308      -19     
==========================================
- Hits        12248    12232      -16     
+ Misses       6079     6076       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

keflavich · 2024-03-16T17:18:29Z

OK, looks like we're green. The readthedocs failure doesn't seem to be associated with this PR, as far as I can see.

keflavich · 2024-03-18T02:02:21Z

@bsipocz this is ready for your review. Any ideas on the readthedocs issue?

bsipocz · 2024-03-18T17:18:47Z

RTD failure is pyvo related and I'll look into it separately.

keflavich · 2024-04-02T11:38:30Z

@bsipocz is there anything holding this back from getting merged?

bsipocz · 2024-04-06T01:49:26Z

@bsipocz is there anything holding this back from getting merged?

Sorry, apparently I have a half finished review here with some minor comments. RTD has been fixed, so that is not holding it up.

keflavich · 2024-04-06T01:50:59Z

Do I need to rebase or rerun tests to get RTD to pass?

bsipocz

There are multiple failures in the RST docs, those needs to be fixed (most of them are cosmetic, but the last ones seem genuine due to the changed column names).

The rest of the inline comments are minor and would be happy to see them addressed, but it's also OK if they are not.

bsipocz · 2024-03-18T17:26:02Z

-                              "specification is overridden")
-            payload['band'] = band
-        elif min_frequency is not None and max_frequency is not None:
+        payload = {"searchSpecies": "",


Is there any way to recognize changes in the required payload, e.g. load these keys from anywhere upstream?

Not as far as I'm aware. It gets populated by a javascript script main.js with this function (below) that we could back out parameters from, but it's not worth my time to figure that out:

getDataDict() { var t = this.advancedLineIntesity , i = 0; return this.advancedLineIntesity.startsWith("CDMS/JPL (log)") ? i = this.advancedLineIntesityCDMS : this.advancedLineIntesity.startsWith("Sij-mu2") ? i = this.advancedLineIntesitySijmu2 : this.advancedLineIntesity.startsWith("Aij (log)") && (i = this.advancedLineIntesityAij), { searchSpecies: this.searchspeciesinput, speciesSelectBox: this.speciesSelectBoxIds, dataVersion: this.advancedDataVersion, userInputFrequenciesFrom: this.FreqRangesFrom, userInputFrequenciesTo: this.FreqRangesTo, userInputFrequenciesUnit: this.addvancedFreqUnit, frequencyRedshift: this.redshiftvalue, energyFrom: this.advancedEnergyFrom, energyTo: this.advancedEnergyTo, energyRangeType: this.advancedEnergyType, lineIntensity: t, lineIntensityLowerLimit: i, excludeAtmosSpecies: this.advancedExAtmosSpecies, excludePotentialInterstellarSpecies: this.advancedExPotInterSpecies, excludeProbableInterstellarSpecies: this.advancedExProbInterSpecies, excludeKnownASTSpecies: this.advancedExASTSpecies, showOnlyAstronomicallyObservedTransitions: this.advancedOnlyObTransition, showOnlyNRAORecommendedFrequencies: this.advancedOnlyNRAORecd, lineListDisplayJPL: this.advancedJPL, lineListDisplayCDMS: this.advancedCDMS, lineListDisplayLovasNIST: this.advancedLoveNIST, lineListDisplaySLAIM: this.advancedSLAIM, lineListDisplayToyaMA: this.advancedToyaMA, lineListDisplayOSU: this.advancedOSU, lineListDisplayRecombination: this.advancedRecomb, lineListDisplayTopModel: this.advancedTopModel, lineListDisplayRFI: this.advancedRFI, lineStrengthDisplayCDMSJPL: this.advancedStrCDMS, lineStrengthDisplaySijMu2: this.advancedStrSimjMu2, lineStrengthDisplaySij: this.advancedStrSij, lineStrengthDisplayAij: this.advancedStrAij, lineStrengthDisplayLovasAST: this.advancedStrLovasAST, energyLevelOne: this.advancedEngEcm1, energyLevelTwo: this.advancedEngEK, energyLevelThree: this.advancedEngEUpcm, energyLevelFour: this.advancedEngEUpK, displayObservedTransitions: this.advancedDispObsTrans, displayG358MaserTransitions: this.advancedDispG358, displayObservationReference: this.advancedDispObsRef, displayObservationSource: this.advancedDispObsSrc, displayTelescopeLovasNIST: this.advancedDispTelLovas, frequencyErrorLimit: this.advancedNoFreqError, displayHFSIntensity: this.advancedDispHFS, displayUnresolvedQuantumNumbers: this.advancedDispUnResQuat, displayUpperStateDegeneracy: this.advancedDispUpSteDegn, displayMoleculeTag: this.advancedDispMoleTag, displayQuantumNumberCode: this.advancedDispQuatCode, displayLabRef: this.advancedDispLabRef, displayOrderedFrequencyOnly: this.advancedDispOrdFreqOnly, displayNRAORecommendedFrequencies: this.advancedDispNRAORecFreq, displayUniqueSpeciesTag: this.advancedDispUnqSpeciesTag, displayUniqueLineIDNumber: this.advancedDispUnqLineID, exportType: this.advancedExportType, exportDelimiter: this.advancedExportDelimiter, exportLimit: this.advancedExportRange, exportStart: this.advancedExportStart, exportStop: this.advancedExportStop } }

bsipocz · 2024-03-18T17:28:04Z

-        elif chemical_name in ('', {}, (), [], set()):
-            # include all
-            payload['sid[]'] = []
+        if chemical_name in ('', {}, (), [], set(), None):


This is a tiny bit hacky (I see that it was like that before). Couldn't we enforce e.g. None as default and then expect the user to provide something none empty? (or this would be filled in from parsing something that results in a ''?)

I think yes? But I am worried about exactly the case you mention, i.e., '' is passed in.

i could simplify this to chemical_name in ('', None), though; the rest of the 'null' options don't really make sense.

bsipocz · 2024-03-18T17:31:58Z

-            Attempt to simplify / clean up the column headers returned by
-            splatalogue to make them more terminal-friendly
+        verbose : bool
+            Has no effect; kept for API compatibility


deprecate it then

bsipocz · 2024-04-06T02:03:43Z

Do I need to rebase or rerun tests to get RTD to pass?

No need to rebase as not much has changed overall, but also wouldn't hurt once you fixed the docs.

bsipocz · 2024-04-07T02:48:25Z

RTD failure is now relevant to this PR.

bsipocz

I'll rebase this to fix the changelog conflict and then merge.

Thanks @keflavich!

whitespace whitespace

fix syntax err whitespace

…column in the returned data)

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

… test...)

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

keflavich marked this pull request as draft February 27, 2024 21:56

keflavich added the splatalogue label Feb 27, 2024

keflavich force-pushed the splatalogue_feb2024 branch from c1dea45 to 08f3244 Compare February 27, 2024 22:04

keflavich force-pushed the splatalogue_feb2024 branch from 2183c9a to 50b634e Compare March 13, 2024 03:30

bsipocz added this to the v0.4.8 milestone Mar 13, 2024

keflavich marked this pull request as ready for review March 16, 2024 15:19

keflavich changed the title ~~WIP: Fix for #2956 (broken splatalogue)~~ Fix for #2956 (broken splatalogue) Mar 18, 2024

bsipocz changed the title ~~Fix for #2956 (broken splatalogue)~~ API: Fix splatalogue module with upstream changes Mar 18, 2024

bsipocz added the changed remote API label Mar 18, 2024

bsipocz requested changes Apr 6, 2024

View reviewed changes

keflavich force-pushed the splatalogue_feb2024 branch from ac4fde0 to e4b3bbd Compare April 6, 2024 18:56

bsipocz reviewed Apr 7, 2024

View reviewed changes

Comment thread docs/splatalogue/splatalogue.rst Outdated

bsipocz approved these changes Apr 9, 2024

View reviewed changes

keflavich added 5 commits April 9, 2024 10:22

WIP: first attempt to fix splatalogue [skip ci]

3da7e65

fix table parsing. Everything works again now?

caf2746

whitespace whitespace

fix some of the broken tests

311ae17

fix keywords invovled in refactor

9ebf6da

add a dictionary that _can_ be used to restore new tables to old format

11e31a9

keflavich and others added 25 commits April 9, 2024 10:22

add fixes for minimize_table and column merging

60af30e

remove unused, unsupported slap code

a97d07a

fix tests for updated utils

d12a7da

fix syntax err whitespace

add changelog entry

91aec4c

fix data in conftest

62cf9b9

fix data: remove old/outdated csv and add json

cb656fa

add more cleanup of html junk

9c47f9d

remove obsolete merge_frequeencies (there is now a unified frequency …

675c377

…column in the returned data)

rename some things

9c20d54

remove "band" keyword argument as it is no longer used or supported

ad414a2

remove "top20" keyword argument as it is no longer used or supported

ceecde0

fix a few more tests

d40a486

finally fixed all tests

090f737

whitespace/style cleanup

4193f93

package .json data

b46834a

fix mocks (did these _ever_ work?)

b9384f9

fix order

35a0324

mocks, etc., are set up in conftest

5e3eebc

codestyle: remove imports

5803174

add another test to increase coverage slightly

3b6adf9

add another test to improve coverage (remote)

09f6211

Remove unnecessary comment

94f3c79

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

docuemntation cleanup, fix to some parameter validation

ddff59f

fix template (it turns out to be kind of a useless file but nice as a…

c5a6d1e

… test...)

Update docs/splatalogue/splatalogue.rst

9f2c437

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

bsipocz force-pushed the splatalogue_feb2024 branch from f102c5f to 9f2c437 Compare April 9, 2024 17:24

bsipocz merged commit 7ffe123 into astropy:main Apr 9, 2024

keflavich mentioned this pull request Jun 6, 2024

Splatalogue.query_lines() seems to be broken? #3024

Closed

Uh oh!

Uh oh!

Conversation

keflavich commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2024-04-09 17:24:19 UTC

Uh oh!

keflavich commented Mar 16, 2024

Uh oh!

keflavich commented Mar 16, 2024

Uh oh!

codecov Bot commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

keflavich commented Mar 16, 2024

Uh oh!

keflavich commented Mar 18, 2024

Uh oh!

bsipocz commented Mar 18, 2024

Uh oh!

keflavich commented Apr 2, 2024

Uh oh!

bsipocz commented Apr 6, 2024

Uh oh!

keflavich commented Apr 6, 2024

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bsipocz Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

keflavich Apr 6, 2024

Choose a reason for hiding this comment

Uh oh!

bsipocz Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

keflavich Apr 6, 2024

Choose a reason for hiding this comment

Uh oh!

bsipocz Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bsipocz commented Apr 6, 2024

Uh oh!

bsipocz commented Apr 7, 2024

Uh oh!

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keflavich commented Feb 27, 2024 •

edited

Loading

pep8speaks commented Feb 27, 2024 •

edited

Loading

codecov Bot commented Mar 16, 2024 •

edited

Loading