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

Remove vendored obsgeo_to_frame in favor of using Astropy #7470

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

ahmedhosssam
Copy link
Contributor

Hi,
I found that some tests in sunpy/coordinates/tests/test_wcs_utils.py should be removed after Astropy 5.0, but Astropy is now 6.0 so I think it should be removed, right?
If it's right, I think it will make the whole testing process faster, now pytest runs in less than 120 seconds after it used to take more than 180 seconds on my computer.

Please let me know if I'm missing something.

@ahmedhosssam ahmedhosssam requested a review from a team as a code owner March 2, 2024 19:28
@ayshih
Copy link
Member

ayshih commented Mar 2, 2024

The vendored version of sunpy.coordinates wcs_utils.obsgeo_to_frame() should be removed at the same time (this all came in with #5315).

It'd be mind-boggling if these three tests take over a minute to run in your environment. I suspect that something went awry in your comparison.

@ahmedhosssam
Copy link
Contributor Author

The vendored version of sunpy.coordinates wcs_utils.obsgeo_to_frame() should be removed at the same time (this all came in with #5315).

Done.
Should I remove these tests also?

@pytest.mark.parametrize("obsgeo", ([np.nan] * 6, None, [0] * 6, [54] * 5))
def test_obsgeo_invalid(obsgeo):

    with pytest.raises(ValueError):
        obsgeo_to_frame(obsgeo, None)
def test_observer_hgln_crln_priority():
    """
    When extracting the observer information from a FITS header, ensure
    Stonyhurst (HG) coordinates are preferred over Carrington (CR) if present.
    Note that `Map` creates a custom FITS header with a sanitized observer
    location, so it is separately tested in the map module.
    """
    header = {'CRVAL1': 0,
              'CRVAL2': 0,
              'CRPIX1': 5,
              'CRPIX2': 5,
              'CDELT1': 10,
              'CDELT2': 10,
              'CUNIT1': 'arcsec',
              'CUNIT2': 'arcsec',
              'PC1_1': 0,
              'PC1_2': -1,
              'PC2_1': 1,
              'PC2_2': 0,
              'NAXIS1': 6,
              'NAXIS2': 6,
              'CTYPE1': 'HPLN-TAN',
              'CTYPE2': 'HPLT-TAN',
              'date-obs': '1970-01-01T00:00:00',
              'mjd-obs': 40587,
              'hglt_obs': 0,
              'hgln_obs': 0,
              'crlt_obs': 2,
              'crln_obs': 2,
              'dsun_obs': 10,
              'rsun_ref': 690000000}
    wcs = WCS(header)
    c = wcs.pixel_to_world(0, 0)
    assert c.observer.lon == 0 * u.deg
    # Note: don't test whether crlt or hglt is used---according to
    # _set_wcs_aux_obs_coord, those are expected to always be the same and so
    # the same one is always used

@nabobalis
Copy link
Contributor

Do you mean test_obsgeo_invalid? I think that should go but I don't see why the test after that would go?

@ahmedhosssam
Copy link
Contributor Author

All of the tests that I removed already exist in astropy.wcs.tests.test_utils.py now, except test_observer_hgln_crln_priority, it tests astropy.wcs.WCS. As far as I understand, the test should belong to astropy rather than sunpy. Or the data in header are related to sunpy?

@nabobalis
Copy link
Contributor

I would need Albert to double check but that test should stay in sunpy.

@wtbarnes wtbarnes changed the title Remove outdated tests Remove vendored obsgeo_to_frame in favor of using Astropy Mar 7, 2024
@ayshih
Copy link
Member

ayshih commented Mar 7, 2024

All of the tests that I removed already exist in astropy.wcs.tests.test_utils.py now, except test_observer_hgln_crln_priority, it tests astropy.wcs.WCS. As far as I understand, the test should belong to astropy rather than sunpy. Or the data in header are related to sunpy?

It's not readily apparent from the test code, but it internally calls a sunpy function to convert the WCS header to a SunPy coordinate frame. The test should stay.

Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

Please make the changes flagged by pre-commit, otherwise looks good to me

@ahmedhosssam
Copy link
Contributor Author

Please make the changes flagged by pre-commit, otherwise looks good to me

Done.

@ayshih ayshih merged commit 07b1637 into sunpy:main Mar 7, 2024
25 of 26 checks passed
@ayshih
Copy link
Member

ayshih commented Mar 7, 2024

Thanks for the PR!

@ahmedhosssam ahmedhosssam deleted the outdated-tests branch March 8, 2024 09:26
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.

None yet

4 participants