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

API: Fix splatalogue module with upstream changes #2960

Merged
merged 30 commits into from
Apr 9, 2024

Conversation

keflavich
Copy link
Contributor

@keflavich keflavich commented Feb 27, 2024

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
Copy link

pep8speaks commented Feb 27, 2024

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 keflavich marked this pull request as draft February 27, 2024 21:56
@keflavich keflavich force-pushed the splatalogue_feb2024 branch from 2183c9a to 50b634e Compare March 13, 2024 03:30
@bsipocz bsipocz added this to the v0.4.8 milestone Mar 13, 2024
@keflavich keflavich marked this pull request as ready for review March 16, 2024 15:19
@keflavich
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link

codecov bot commented Mar 16, 2024

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
Copy link
Contributor Author

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 keflavich changed the title WIP: Fix for #2956 (broken splatalogue) Fix for #2956 (broken splatalogue) Mar 18, 2024
@keflavich
Copy link
Contributor Author

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

@bsipocz
Copy link
Member

bsipocz commented Mar 18, 2024

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

@bsipocz bsipocz changed the title Fix for #2956 (broken splatalogue) API: Fix splatalogue module with upstream changes Mar 18, 2024
@keflavich
Copy link
Contributor Author

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

@bsipocz
Copy link
Member

bsipocz commented Apr 6, 2024

@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
Copy link
Contributor Author

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

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

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.

"specification is overridden")
payload['band'] = band
elif min_frequency is not None and max_frequency is not None:
payload = {"searchSpecies": "",
Copy link
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?

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

elif chemical_name in ('', {}, (), [], set()):
# include all
payload['sid[]'] = []
if chemical_name in ('', {}, (), [], set(), None):
Copy link
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 ''?)

Copy link
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.

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
Copy link
Member

Choose a reason for hiding this comment

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

deprecate it then

@bsipocz
Copy link
Member

bsipocz commented Apr 6, 2024

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.

@keflavich keflavich force-pushed the splatalogue_feb2024 branch from ac4fde0 to e4b3bbd Compare April 6, 2024 18:56
@bsipocz
Copy link
Member

bsipocz commented Apr 7, 2024

RTD failure is now relevant to this PR.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

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

Thanks @keflavich!

@bsipocz bsipocz force-pushed the splatalogue_feb2024 branch from f102c5f to 9f2c437 Compare April 9, 2024 17:24
@bsipocz bsipocz merged commit 7ffe123 into astropy:main Apr 9, 2024
8 checks passed
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.

New Splatalogue breaks astroquery interface
3 participants