Skip to content

API: Fix splatalogue module with upstream changes #2960

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.

Sign up for GitHub

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

Merged
bsipocz merged 30 commits into from
Apr 9, 2024
Merged

API: Fix splatalogue module with upstream changes #2960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3da7e65
WIP: first attempt to fix splatalogue [skip ci]
keflavich Feb 27, 2024
caf2746
fix table parsing. Everything works again now?
keflavich Feb 27, 2024
311ae17
fix some of the broken tests
keflavich Feb 27, 2024
9ebf6da
fix keywords invovled in refactor
keflavich Feb 28, 2024
11e31a9
add a dictionary that _can_ be used to restore new tables to old format
keflavich Feb 29, 2024
60af30e
add fixes for minimize_table and column merging
keflavich Mar 5, 2024
a97d07a
remove unused, unsupported slap code
keflavich Mar 5, 2024
d12a7da
fix tests for updated utils
keflavich Mar 5, 2024
91aec4c
add changelog entry
keflavich Mar 13, 2024
62cf9b9
fix data in conftest
keflavich Mar 13, 2024
cb656fa
fix data: remove old/outdated csv and add json
keflavich Mar 13, 2024
9c47f9d
add more cleanup of html junk
keflavich Mar 13, 2024
675c377
remove obsolete merge_frequeencies (there is now a unified frequency …
keflavich Mar 13, 2024
9c20d54
rename some things
keflavich Mar 14, 2024
ad414a2
remove "band" keyword argument as it is no longer used or supported
keflavich Mar 16, 2024
ceecde0
remove "top20" keyword argument as it is no longer used or supported
keflavich Mar 16, 2024
d40a486
fix a few more tests
keflavich Mar 16, 2024
090f737
finally fixed all tests
keflavich Mar 16, 2024
4193f93
whitespace/style cleanup
keflavich Mar 16, 2024
b46834a
package .json data
keflavich Mar 16, 2024
b9384f9
fix mocks (did these _ever_ work?)
keflavich Mar 16, 2024
35a0324
fix order
keflavich Mar 16, 2024
5e3eebc
mocks, etc., are set up in conftest
keflavich Mar 16, 2024
5803174
codestyle: remove imports
keflavich Mar 16, 2024
3b6adf9
add another test to increase coverage slightly
keflavich Mar 16, 2024
09f6211
add another test to improve coverage (remote)
keflavich Mar 16, 2024
94f3c79
Remove unnecessary comment
keflavich Apr 6, 2024
ddff59f
docuemntation cleanup, fix to some parameter validation
keflavich Apr 6, 2024
c5a6d1e
fix template (it turns out to be kind of a useless file but nice as a…
keflavich Apr 6, 2024
9f2c437
Update docs/splatalogue/splatalogue.rst
keflavich Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
finally fixed all tests
  • Loading branch information
@keflavich @bsipocz
keflavich authored and bsipocz committed Apr 9, 2024
commit 090f737701f242559e3058b2cd99e4861cb39296
12 changes: 5 additions & 7 deletions astroquery/splatalogue/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,6 @@ def _parse_kwargs(self, *, min_frequency=None, max_frequency=None,

"""

payload = {'submit': 'Search',
'frequency_units': 'GHz',
}

payload = {"searchSpecies": "",

@bsipocz bsipocz Mar 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@keflavich keflavich Apr 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
                    }
                }

"speciesSelectBox": [],
"dataVersion": "v3.0",
Expand Down Expand Up @@ -346,9 +342,11 @@ def _parse_kwargs(self, *, min_frequency=None, max_frequency=None,
payload['userInputFrequenciesFrom'] = [min_frequency.value]
payload['userInputFrequenciesTo'] = [max_frequency.value]

if chemical_name in ('', {}, (), [], set()):
# include all
payload['speciesSelectBox'] = []
if chemical_name in ('', {}, (), [], set(), None):

@bsipocz bsipocz Mar 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ''?)

@keflavich keflavich Apr 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

# include all by default, or whatever default was set
payload['speciesSelectBox'] = (self.data['speciesSelectBox']
if hasattr(self, 'data')
else [])
elif chemical_name is not None:
if parse_chemistry_locally:
species_ids = self.get_species_ids(species_regex=chemical_name, reflags=chem_re_flags)
Expand Down
17 changes: 14 additions & 3 deletions astroquery/splatalogue/tests/test_splatalogue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def test_simple(patch_post):


@pytest.mark.remote_data
def test_init(patch_post):
def test_init_remote(patch_post):
x = splatalogue.Splatalogue.query_lines(min_frequency=114 * u.GHz,
max_frequency=116 * u.GHz,
chemical_name=' CO ')
Expand All @@ -34,10 +34,21 @@ def test_init(patch_post):
# masked arrays fail
# assert y == x
assert len(x) == len(y)
assert all(y['Species'] == x['Species'])
assert all(x['Chemical Name'] == y['Chemical Name'])
assert all(y['species_id'] == x['species_id'])
assert all(y['name'] == x['name'])
assert all(y['chemical_name'] == x['chemical_name'])


def test_init():
splat = splatalogue.Splatalogue(chemical_name=' CO ')
assert splat.data['speciesSelectBox'] == ['204', '990', '991', '1343']
payload = splat.query_lines(min_frequency=114 * u.GHz, max_frequency=116 * u.GHz,
get_query_payload=True)
payload = json.loads(payload['body'])
assert payload['speciesSelectBox'] == ['204', '990', '991', '1343']
assert payload['userInputFrequenciesFrom'] == [114.0]
assert payload['userInputFrequenciesTo'] == [116.0]

def test_load_species_table():
tbl = splatalogue.load_species_table.species_lookuptable()
CO = tbl.find(' CO ')
Expand Down
8 changes: 5 additions & 3 deletions astroquery/splatalogue/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ def test_minimize(patch_post):

@pytest.mark.remote_data
def test_minimize_issue2135():
"""
This was a regression test for 2135, but is now just a basic test for the
new (March 2024) keywords
"""
rslt = splatalogue.Splatalogue.query_lines(min_frequency=100*u.GHz,
max_frequency=200*u.GHz,
chemical_name=' SiO ',
Expand All @@ -40,6 +44,4 @@ def test_minimize_issue2135():

minimized = utils.minimize_table(rslt)

theomask = rslt['orderedFreq'].mask
np.testing.assert_allclose(minimized['Freq'][theomask],
rslt['measFreq'][theomask])
np.testing.assert_allclose(minimized['Freq'], rslt['orderedfreq'])