increase min sunpy #63

nabobalis · 2024-07-23T23:27:07Z

This supersedes #48

Fixes Add towncrier #49
Renamed FortranTracer to PerformanceTracer
Deprecated FortranTracer
streamtracer is now a hard dep
Deprecated bunch of utils I think we don't need anymore.
Bunch of package updates
Trying to tidy up the example gallery
Adaptmap removed due to upstream support in sunpy

This turned into a large PR. I could split it out but i'm lazy.

sunkit_magex/pfss/tests/test_pfss.py

nabobalis · 2024-07-24T01:18:34Z

sunkit_magex/pfss/utils.py

-        hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))
-
-
-def load_adapt(adapt_path):


We dont have the same loader in sunpy but we can now load them directly. But only the first one.

I don't know enough about the data to know if the other ones are useful, but if there isn't a drop in replacement then maybe we shouldn't yank it?

Also deprecations please 👼

I personally use load_adapt almost every day i do research with sunkit-magex and send people examples which use it. It's very useful to be able to in one line get all the realizations loaded into a Map and then decide what to do next (choose one, average them all etc.), so i'd advocate that this function would be useful to have somewhere. I get that handling specific magnetograms doesn't make sense to be the job of sunkit-magex - could it be possible to house it in sunpy.map ? Is the direct use of astropy.io.fits problematic?

In any case, I'm good with this being deprecated for now and having this discussion elsewhere. Is it bad practise to have a long message here. if not, I suggest :

message = "Use sunpy.map.Map to load the first realization (see <link to examples/utils/reproject_car_to_cea.py>) or see https://docs.sunpy.org/en/latest/generated/gallery/saving_and_loading_data/load_adapt_fits_into_map.html on how to load all realizations"

Cadair

Thanks @nabobalis

I think we need to be deprecating stuff, I made the first version 1.0.0 for a reason, I think stability is important here.

I will let @jgieseler and @STBadman chime in on things like load_adapt and fix_hmi_meta specifically, but I would like a review from at least one of them before we merge.

docs/installing.rst

pyproject.toml

sunkit_magex/pfss/tests/test_tracers.py

sunkit_magex/pfss/tracing.py

Cadair · 2024-07-24T08:38:14Z

sunkit_magex/pfss/utils.py

-        hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))
-
-
-def load_adapt(adapt_path):


I don't know enough about the data to know if the other ones are useful, but if there isn't a drop in replacement then maybe we shouldn't yank it?

Also deprecations please 👼

examples/using_sunkit_magex_pfss/overplotting_onto_aia.py

nabobalis · 2024-07-25T19:52:25Z

sunkit_magex/pfss/input.py

@@ -42,7 +42,6 @@ def __init__(self, br, nr, rss):
        sunkit_magex.pfss.utils.is_full_sun_synoptic_map(br, error=True)

        self._map_in = copy.deepcopy(br)
-        self.dtime = self.map.date


Not sure if we need this but it should be reference_date if we do.

I'd probably not remove this property from the Input class, so 👍 making it reference_date.

I think this property might originally have been needed when the input and output had individual plot methods, i think that was removed since they contain maps with their own plot methods. I'd be ok dropping it as proposed here if nothing breaks.

nabobalis · 2024-07-25T19:52:58Z

I need to patch sunpy 6.0.0 first and then this should pass.

jgieseler · 2024-09-17T15:07:20Z

Yes, I saw that comment as well and was confused. That's why I was wondering if I do the loading in a wrong way.

nabobalis · 2024-09-17T15:11:07Z

Oh I am confused, sunpy doesn't change the meta anymore. If you access the attributes of the map, those should be correct now. I think .units?

jgieseler · 2024-09-17T15:24:04Z

Oh I am confused, sunpy doesn't change the meta anymore. If you access the attributes of the map, those should be correct now. I think .units?

Hm, I don't know. But I just checked and run some old code that uses fix_hmi_meta. I run it with applying fix_hmi_meta and also without (i.e., just loading the HMI file with sunpy). The resulting figures look the same, only in the latter case, some SunpyMetadataWarning are printed:

Missing metadata for solar radius: assuming the standard radius of the photosphere. [sunpy.map.mapbase]
2024-09-17 18:19:50 - sunpy - INFO: Missing metadata for solar radius: assuming the standard radius of the photosphere.
WARNING: SunpyMetadataWarning: Missing metadata for observer: assuming Earth-based observer.
For frame 'heliographic_stonyhurst' the following metadata is missing: hgln_obs,hglt_obs,dsun_obs
For frame 'heliographic_carrington' the following metadata is missing: crlt_obs,dsun_obs,crln_obs
 [sunpy.map.mapbase]
2024-09-17 18:19:50 - sunpy - WARNING: SunpyMetadataWarning: Missing metadata for observer: assuming Earth-based observer.
For frame 'heliographic_stonyhurst' the following metadata is missing: hgln_obs,hglt_obs,dsun_obs
For frame 'heliographic_carrington' the following metadata is missing: crlt_obs,dsun_obs,crln_obs

Fixed

nabobalis · 2024-11-11T22:53:40Z

Oh I am confused, sunpy doesn't change the meta anymore. If you access the attributes of the map, those should be correct now. I think .units?

Hm, I don't know. But I just checked and run some old code that uses fix_hmi_meta. I run it with applying fix_hmi_meta and also without (i.e., just loading the HMI file with sunpy). The resulting figures look the same, only in the latter case, some SunpyMetadataWarning are printed:

Missing metadata for solar radius: assuming the standard radius of the photosphere. [sunpy.map.mapbase]
2024-09-17 18:19:50 - sunpy - INFO: Missing metadata for solar radius: assuming the standard radius of the photosphere.
WARNING: SunpyMetadataWarning: Missing metadata for observer: assuming Earth-based observer.
For frame 'heliographic_stonyhurst' the following metadata is missing: hgln_obs,hglt_obs,dsun_obs
For frame 'heliographic_carrington' the following metadata is missing: crlt_obs,dsun_obs,crln_obs
 [sunpy.map.mapbase]
2024-09-17 18:19:50 - sunpy - WARNING: SunpyMetadataWarning: Missing metadata for observer: assuming Earth-based observer.
For frame 'heliographic_stonyhurst' the following metadata is missing: hgln_obs,hglt_obs,dsun_obs
For frame 'heliographic_carrington' the following metadata is missing: crlt_obs,dsun_obs,crln_obs

Ah I see, yeah but we should not be fixing these metadata ourselves but letting sunpy emit them. These are legit issues with the FITS files and having some special helper function for it is not something we should be doing in sunkit-magex.

nabobalis · 2024-12-30T17:10:00Z

Dob build is failing due to JSOC. I can skip the examples.

nabobalis · 2024-12-30T17:15:42Z

sunkit_magex/pfss/utils.py

@@ -36,6 +37,7 @@ def _earth_obs_coord_meta(obstime):
    return _observer_coord_meta(sunpy.coordinates.get_earth(obstime))


+@deprecated('1.0', message="This is now fixed within sunpy when you load a HMI file.")
 def fix_hmi_meta(hmi_map):


So the only fix not in sunpy core is:

# Fix observer coordinate if 'hglt_obs' not in hmi_map.meta: hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))

We can talk about adding it but that becomes an open question for the other SDO or earth based observatories.

I'm ok with this being deprecated but tracking the discussion elsewhere

Cadair · 2025-02-04T14:21:03Z

changelog/63.breaking.2.rst

+- pfss.utils.fix_hmi_meta
+- pfss.utils.load_adapt
+
+These functions are no longer needed and will be removed.


Are there alternatives we can direct people to?

There is an example on how to load the adapt maps. FOr the fix meta, we have the how to.

I can link to both.

Cadair · 2025-02-04T14:24:31Z

sunkit_magex/pfss/utils.py

@@ -79,6 +81,7 @@ def fix_hmi_meta(hmi_map):
        hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))


+@deprecated('1.0', message="This is not required anymore. An example of how to load an ADAPT file is available in the documentation.")


I'd either link to it or say use sunpy.map.Map or similar.

Cadair · 2025-02-04T14:27:34Z

I'm generally happy with this and we should ship a release, so if we can get the CI fixed and one of @jgieseler or @STBadman are ok with this we should merge it.

STBadman · 2025-02-04T15:02:16Z

Sorry, this has been on my to-do list to review for ages - just reading the conversation I think I agree with the changes but I'd like to try out a workflow with ADAPT and HMI and make sure I understand it all. I'm at a workshop first half of this week but I will have time on Thursday if you're ok to wait till then.

…

On Tue, Feb 4, 2025, 09:27 Stuart Mumford ***@***.***> wrote: I'm generally happy with this and we should ship a release, so if we can get the CI fixed and one of @jgieseler <https://github.com/jgieseler> or @STBadman <https://github.com/STBadman> are ok with this we should merge it. — Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANC6CA4U3A5ARJCLXRLS7A32ODE63AVCNFSM6AAAAABLLLNBVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZUGEZDQNJZGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Cadair · 2025-02-04T15:39:49Z

yep, that's fine no rush!

nabobalis · 2025-02-05T00:20:50Z

pyproject.toml

+  "scipy>=1.10.1",
+  "streamtracer>=2.2.0",
+  "sunpy[map]>=6.0.1,!=6.0.0",
+  "matplotlib>=3.5.0",  # Needed to solve the correct env for oldestdeps


I didn't know how else to handle it

nabobalis · 2025-02-05T00:22:34Z

sunkit_magex/pfss/analytic.py

+    if hasattr(scipy.special, "sph_harm_y"):
+        return -scipy.special.sph_harm_y(l, m, phi, theta)
+    if hasattr(scipy.special, "sph_harm"):
+        return -scipy.special.sph_harm(m, l, phi, theta)


I think for the new version (sph_harm_y) theta and phi should be swapped as well. in the old version theta was in the exponent, phi was in the Pn^m term, now swapped.

if hasattr(scipy.special, "sph_harm_y"): return -scipy.special.sph_harm_y(l, m, theta, phi) if hasattr(scipy.special, "sph_harm"): return -scipy.special.sph_harm(m, l, phi, theta)

nabobalis · 2025-02-05T00:23:06Z

sunkit_magex/pfss/tests/test_analytic.py

    m = 0
    c = zss**(-l-2) * ((2*l + 1) / (l + 1 + l * zss**(-2*l - 1)))
    f = analytic.Br(l, m, zss)
    phi = 0 * u.deg
    theta = 0 * u.deg
-    assert f(zss, theta, phi) == -0.5 * np.sqrt(3 / np.pi) * c
+    np.testing.assert_allclose(f(zss, theta, phi), -0.5 * np.sqrt(3 / np.pi) * c)


There is a floating point difference now at the 8th decime.

I think this is fine, it is probably scientifically something interesting to understand (uncertainty in field line tracing) but nothing to worry about for the code.

STBadman

Almost everything looks good except the deprecation of FortranTracer looks to be broken - it's inheriting a child Class and as I understand it, this needs to be passed to .super()

Otherwise, I'm ok with load_adapt and fix_hmi_meta being deprecated and discussion of any gaps in capability moving to sunpy.

Thanks for all this work!

STBadman · 2025-02-07T15:49:36Z

sunkit_magex/pfss/tracing.py

+class FortranTracer(PerformanceTracer):
+    @deprecated('1.0', message='This class has been renamed to PerformanceTracer as this now does not use Fortran.')
+    def __init__(*args, **kwargs):
+        super().__init__(*args, **kwargs)


I think super() needs to be given PerformanceTracer as an argument (https://www.geeksforgeeks.org/python-runtimeerror-super-no-arguments/)

(i see the deprecation warning but then i get an error that super has no args)

STBadman · 2025-02-07T15:52:32Z

sunkit_magex/pfss/tests/test_analytic.py

    m = 0
    c = zss**(-l-2) * ((2*l + 1) / (l + 1 + l * zss**(-2*l - 1)))
    f = analytic.Br(l, m, zss)
    phi = 0 * u.deg
    theta = 0 * u.deg
-    assert f(zss, theta, phi) == -0.5 * np.sqrt(3 / np.pi) * c
+    np.testing.assert_allclose(f(zss, theta, phi), -0.5 * np.sqrt(3 / np.pi) * c)


I think this is fine, it is probably scientifically something interesting to understand (uncertainty in field line tracing) but nothing to worry about for the code.

STBadman · 2025-02-07T15:53:58Z

sunkit_magex/pfss/input.py

@@ -42,7 +42,6 @@ def __init__(self, br, nr, rss):
        sunkit_magex.pfss.utils.is_full_sun_synoptic_map(br, error=True)

        self._map_in = copy.deepcopy(br)
-        self.dtime = self.map.date


I think this property might originally have been needed when the input and output had individual plot methods, i think that was removed since they contain maps with their own plot methods. I'd be ok dropping it as proposed here if nothing breaks.

STBadman · 2025-02-07T16:02:32Z

docs/performance.rst

@@ -11,9 +11,8 @@ To enable this simply `install numba`_  and use `sunkit_magex.pfss` as normal.
 Streamline tracing
 ==================

-`sunkit_magex.pfss` has two streamline tracers: a pure python `sunkit_magex.pfss.tracing.PythonTracer` and a complied tracer `sunkit_magex.pfss.tracing.FortranTracer`.
-The compiled version is significantly faster, using the `streamtracer`_ package.
+`sunkit_magex.pfss` uses a complied tracer `sunkit_magex.pfss.tracing.PerformanceTracer` using the `streamtracer`_ package.


I think since we're not removing the pythontracer and there isn't a default behavior implemented anywhere yet we should still mention it here, like it was originally but just change FortranTracer to PerformanceTracer in the old line 14,

STBadman · 2025-02-07T16:08:45Z

examples/testing/field_line_error_map.py

@@ -7,13 +7,13 @@
 """
 import matplotlib.pyplot as plt
 import numpy as np
+from _helpers import pffspy_output, phi_fline_coords, theta_fline_coords


how much of a headache would correcting the typo pffspy_output -> pfsspy_output be? Maybe i can do a separate PR

STBadman · 2025-02-07T16:36:36Z

sunkit_magex/pfss/analytic.py

+    if hasattr(scipy.special, "sph_harm_y"):
+        return -scipy.special.sph_harm_y(l, m, phi, theta)
+    if hasattr(scipy.special, "sph_harm"):
+        return -scipy.special.sph_harm(m, l, phi, theta)


I think for the new version (sph_harm_y) theta and phi should be swapped as well. in the old version theta was in the exponent, phi was in the Pn^m term, now swapped.

if hasattr(scipy.special, "sph_harm_y"): return -scipy.special.sph_harm_y(l, m, theta, phi) if hasattr(scipy.special, "sph_harm"): return -scipy.special.sph_harm(m, l, phi, theta)

STBadman · 2025-02-07T17:40:27Z

sunkit_magex/pfss/utils.py

-        hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))
-
-
-def load_adapt(adapt_path):


I personally use load_adapt almost every day i do research with sunkit-magex and send people examples which use it. It's very useful to be able to in one line get all the realizations loaded into a Map and then decide what to do next (choose one, average them all etc.), so i'd advocate that this function would be useful to have somewhere. I get that handling specific magnetograms doesn't make sense to be the job of sunkit-magex - could it be possible to house it in sunpy.map ? Is the direct use of astropy.io.fits problematic?

In any case, I'm good with this being deprecated for now and having this discussion elsewhere. Is it bad practise to have a long message here. if not, I suggest :

message = "Use sunpy.map.Map to load the first realization (see <link to examples/utils/reproject_car_to_cea.py>) or see https://docs.sunpy.org/en/latest/generated/gallery/saving_and_loading_data/load_adapt_fits_into_map.html on how to load all realizations"

STBadman · 2025-02-07T17:42:30Z

sunkit_magex/pfss/utils.py

@@ -36,6 +37,7 @@ def _earth_obs_coord_meta(obstime):
    return _observer_coord_meta(sunpy.coordinates.get_earth(obstime))


+@deprecated('1.0', message="This is now fixed within sunpy when you load a HMI file.")
 def fix_hmi_meta(hmi_map):


I'm ok with this being deprecated but tracking the discussion elsewhere

nabobalis force-pushed the sunpy_version branch 3 times, most recently from 3b12579 to d4d8d35 Compare July 24, 2024 00:09

nabobalis marked this pull request as draft July 24, 2024 00:09

nabobalis marked this pull request as ready for review July 24, 2024 00:10

nabobalis marked this pull request as draft July 24, 2024 00:10

nabobalis force-pushed the sunpy_version branch 4 times, most recently from 1ba30c9 to d28706f Compare July 24, 2024 01:17

nabobalis commented Jul 24, 2024

View reviewed changes

sunkit_magex/pfss/tests/test_pfss.py Show resolved Hide resolved

nabobalis commented Jul 24, 2024

View reviewed changes

sunkit_magex/pfss/tests/test_pfss.py Show resolved Hide resolved

nabobalis commented Jul 24, 2024

View reviewed changes

nabobalis force-pushed the sunpy_version branch from d28706f to 23a1392 Compare July 24, 2024 01:53

Cadair previously requested changes Jul 24, 2024

View reviewed changes

nabobalis force-pushed the sunpy_version branch 5 times, most recently from 6af00f1 to bfacbe8 Compare July 24, 2024 22:44

nabobalis commented Jul 24, 2024

View reviewed changes

examples/using_sunkit_magex_pfss/overplotting_onto_aia.py Outdated Show resolved Hide resolved

nabobalis force-pushed the sunpy_version branch 5 times, most recently from 2d7c5db to 1f03469 Compare July 25, 2024 19:51

nabobalis commented Jul 25, 2024

View reviewed changes

nabobalis marked this pull request as ready for review July 25, 2024 19:52

nabobalis requested a review from Cadair August 22, 2024 19:54

nabobalis mentioned this pull request Nov 11, 2024

Unable to load ADAPT map with sunpy>=6.0.0 #66

Closed

nabobalis marked this pull request as draft December 30, 2024 16:52

nabobalis force-pushed the sunpy_version branch from de8000d to d57aef3 Compare December 30, 2024 17:03

nabobalis marked this pull request as ready for review December 30, 2024 17:05

nabobalis commented Dec 30, 2024

View reviewed changes

Cadair reviewed Feb 4, 2025

View reviewed changes

nabobalis added 3 commits February 4, 2025 14:06

increase min sunpy

b420bc1

Handle old functions and update examples

9514b3c

bug in sunpy 6.0.0 means we need to wait for bugfix release

03d5661

nabobalis force-pushed the sunpy_version branch 2 times, most recently from b6256f4 to 5eb8e40 Compare February 4, 2025 22:12

CR

9552abf

nabobalis force-pushed the sunpy_version branch from 5eb8e40 to 9552abf Compare February 4, 2025 22:14

nabobalis added 4 commits February 4, 2025 15:31

Fix scipy dep warning

866743a

Update URL for example

c8e712f

fix oldestdeps

a417721

Actually fix example

7a178f9

nabobalis commented Feb 5, 2025

View reviewed changes

STBadman requested changes Feb 7, 2025

View reviewed changes

		hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))


		def load_adapt(adapt_path):

		@@ -79,6 +81,7 @@ def fix_hmi_meta(hmi_map):
		hmi_map.meta.update(_earth_obs_coord_meta(hmi_map.meta['date-obs']))


		@deprecated('1.0', message="This is not required anymore. An example of how to load an ADAPT file is available in the documentation.")

increase min sunpy #63

Are you sure you want to change the base?

increase min sunpy #63

Conversation

nabobalis commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Jul 25, 2024

jgieseler commented Sep 17, 2024

nabobalis commented Sep 17, 2024

jgieseler commented Sep 17, 2024

nabobalis commented Nov 11, 2024

nabobalis commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair commented Feb 4, 2025

STBadman commented Feb 4, 2025 via email

Cadair commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

STBadman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Jul 23, 2024 •

edited

Loading