-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Conversation
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 |
c1dea45
to
08f3244
Compare
2183c9a
to
50b634e
Compare
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 |
ach, nevermind my previous comment; I didn't realize where the mocks were defined (they're in |
Codecov ReportAttention: Patch coverage is
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. |
OK, looks like we're green. The readthedocs failure doesn't seem to be associated with this PR, as far as I can see. |
@bsipocz this is ready for your review. Any ideas on the readthedocs issue? |
RTD failure is pyvo related and I'll look into it separately. |
@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. |
Do I need to rebase or rerun tests to get RTD to pass? |
There was a problem hiding this 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": "", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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 ''
?)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecate it then
No need to rebase as not much has changed overall, but also wouldn't hurt once you fixed the docs. |
ac4fde0
to
e4b3bbd
Compare
RTD failure is now relevant to this PR. |
There was a problem hiding this 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!
fix syntax err whitespace
…column in the returned data)
Co-authored-by: Brigitta Sipőcz <[email protected]>
Co-authored-by: Brigitta Sipőcz <[email protected]>
f102c5f
to
9f2c437
Compare
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