Skip to content

Re-enable high precision test for new versions of jplephem#11700

Merged
pllim merged 1 commit intoastropy:mainfrom
mhvk:coordinates-precision
May 6, 2021
Merged

Re-enable high precision test for new versions of jplephem#11700
pllim merged 1 commit intoastropy:mainfrom
mhvk:coordinates-precision

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented May 5, 2021

Note the slightly inelegant version test for jplephem. Obviously, we could also just require >=2.15, which was released last November. But it would only be for this one precision test, which seems a bit excessive.

Since we know things work, and this will test the main branch, i set the milestone to 5.0.

fixes #11683

@mhvk mhvk added this to the v5.0 milestone May 5, 2021
@mhvk mhvk mentioned this pull request May 5, 2021
Closed
mkbrewer
mkbrewer reviewed May 5, 2021
View reviewed changes
astropy/coordinates/tests/test_intermediate_transformations.py Outdated
# Pre-2.15, some versions of jplephem suffer from time rounding errors.
# We check whether we are up to date by looking for the entry in the
# changelog, which is included in the docstring.
if '**2020 September 2 — Version 2.15**' in jplephem.__doc__:
Copy link
Contributor

@mkbrewer mkbrewer May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with doing this is that if jplephem is ever updated to 2.16, the test will revert to the higher tolerances.

Copy link
Contributor Author

@mhvk mhvk May 5, 2021

Choose a reason for hiding this comment

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

No, the docstring contains entries for all versions, so in that sense it is OK. But I think it should be rewritten to use the version information regardless, as suggested in #11683 (comment) (will have to see if it can be done without pkg_resources)

Copy link
Contributor

@mkbrewer mkbrewer May 5, 2021

Choose a reason for hiding this comment

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

Oh, OK. I didn't realize that.

Copy link
Member

@pllim pllim May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why is jplephem.__version__ not a thing? 😱 (p.s. Ah, I see that this has been discussed. I missed it.)

Is the version embedded in __doc__ predictable? If so, we can probably extract it with regex and then use the normal version comparison. I think we can xfail for 2.14 instead of adding an if-else?

@pllim pllim mentioned this pull request May 5, 2021
Closed
@pllim pllim added the testing label May 5, 2021
@mhvk mhvk force-pushed the coordinates-precision branch from 1753bc7 to 87f2919 Compare May 5, 2021 23:37
@mhvk mhvk force-pushed the coordinates-precision branch from 87f2919 to 26e7398 Compare May 5, 2021 23:38
@mhvk
Copy link
Contributor Author

mhvk commented May 5, 2021

OK, now using importlib.metadata to get the version, and just skip the test altogether for version < 2.15. Should be good now!

@mhvk mhvk mentioned this pull request May 5, 2021
Closed
pllim
pllim approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@pllim pllim merged commit 3eefe03 into astropy:main May 6, 2021
@mhvk mhvk deleted the coordinates-precision branch May 6, 2021 01:34
@pllim
Copy link
Member

pllim commented May 6, 2021

I think this broke pyinstaller job: https://github.com/astropy/astropy/runs/2514846688?check_suite_focus=true

_ ERROR collecting .pyinstaller/astropy_tests/coordinates/tests/test_intermediate_transformations.py _
astropy_tests/coordinates/tests/test_intermediate_transformations.py:687: in <module>
    ???
importlib/metadata.py:530: in version
    ???
importlib/metadata.py:503: in distribution
    ???
importlib/metadata.py:177: in from_name
    ???
E   importlib.metadata.PackageNotFoundError: jplephem
_ ERROR collecting .pyinstaller/astropy_tests/coordinates/tests/test_intermediate_transformations.py _
ImportError while importing test module '/home/runner/work/astropy/astropy/.pyinstaller/astropy_tests/coordinates/tests/test_intermediate_transformations.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
_pytest/python.py:578: in _importtestmodule
    ???
_pytest/pathlib.py:524: in import_path
    ???
importlib/__init__.py:127: in import_module
    ???
_pytest/assertion/rewrite.py:170: in exec_module
    ???
astropy_tests/coordinates/tests/test_intermediate_transformations.py:687: in <module>
    ???
importlib/metadata.py:530: in version
    ???
importlib/metadata.py:503: in distribution
    ???
importlib/metadata.py:177: in from_name
    ???
E   importlib.metadata.PackageNotFoundError: jplephem

@mhvk
Copy link
Contributor Author

mhvk commented May 6, 2021

?!?!? How does metadata.version get executed and not find jplehem when HAS_JPLEPHEM is true?!

@mhvk
Copy link
Contributor Author

mhvk commented May 6, 2021

And, sadly, I cannot reproduce since tox -e pyinstaller just fails with obscure compilation errors (as does tox -e test-oldestdeps - the promise of being able to reproduce problems with tox has not really panned out, unfortunately).

@mhvk
Copy link
Contributor Author

mhvk commented May 6, 2021 •
edited
Loading

More positively, maybe best to move this in some form of try/except, say

def check_jplephem():
    try:
        return HAS_JPLEPHEM and metadata.version('jplephem') >= 2.15
    except Exception:
        return False

What do you think, @pllim?

@pllim
Copy link
Member

pllim commented May 6, 2021

Let's see if @saimn has anything to add, since he is the importlib expert here.

@saimn
Copy link
Contributor

saimn commented May 6, 2021 •
edited
Loading

No idea how jplephem can be importable but importlib.metadata doesn't found it... pyinstaller seems to frequently cause weird issues. Maybe move the importlib.metadata check in the test function and pytest.skip if the version is not correct ?

@mhvk mhvk mentioned this pull request May 6, 2021
Merged
@pllim pllim mentioned this pull request Jun 22, 2021
Closed
pllim added a commit to pllim/astropy that referenced this pull request Jun 22, 2021
@pllim
Merge pull request astropy#11700 from mhvk/coordinates-precision
afb4cdf 
Re-enable high precision test for new versions of jplephem
@pllim pllim mentioned this pull request Jun 22, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes

+1 more reviewer

@mkbrewer mkbrewer mkbrewer left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coordinates no-changelog-entry-needed testing

Projects

None yet

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

Test failure in test_aa_high_precision()

4 participants

@mhvk @pllim @saimn @mkbrewer