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

increase min sunpy #63

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

increase min sunpy #63

wants to merge 6 commits into from

Conversation

nabobalis
Copy link
Contributor

@nabobalis nabobalis commented Jul 23, 2024

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.

@nabobalis nabobalis force-pushed the sunpy_version branch 3 times, most recently from 3b12579 to d4d8d35 Compare July 24, 2024 00:09
@nabobalis nabobalis marked this pull request as draft July 24, 2024 00:09
@nabobalis nabobalis marked this pull request as ready for review July 24, 2024 00:10
@nabobalis nabobalis marked this pull request as draft July 24, 2024 00:10
@nabobalis nabobalis force-pushed the sunpy_version branch 4 times, most recently from 1ba30c9 to d28706f Compare July 24, 2024 01:17
sunkit_magex/pfss/utils.py Outdated Show resolved Hide resolved
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 plan to add this to sunpy instead.

Cadair
Cadair previously requested changes Jul 24, 2024
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
sunkit_magex/pfss/tests/test_tracers.py Outdated Show resolved Hide resolved
sunkit_magex/pfss/tracing.py Outdated Show resolved Hide resolved
sunkit_magex/pfss/utils.py Show resolved Hide resolved
@nabobalis nabobalis force-pushed the sunpy_version branch 4 times, most recently from 6af00f1 to bfacbe8 Compare July 24, 2024 22:44
@nabobalis nabobalis force-pushed the sunpy_version branch 3 times, most recently from 1025051 to e174672 Compare July 24, 2024 23:28
@@ -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
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 sure if we need this but it should be reference_date if we do.

@nabobalis nabobalis marked this pull request as ready for review July 25, 2024 19:52
@nabobalis
Copy link
Contributor Author

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

@nabobalis nabobalis requested a review from Cadair August 22, 2024 19:54
@nabobalis
Copy link
Contributor Author

It is finally passing again with the fix from upstream.

@jgieseler
Copy link
Member

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.

Sorry for the delay, I'm buried in project work atm. 😬 I haven't ever used load_adapt, and fix_hmi_meta not in a long while, so maybe @STBadman can also give some input.

But I'm a bit confused regarding fix_hmi_meta. You state that it's deprecated because this is now fixed within sunpy when you load a HMI file. But if I download a HMI file (with files = Fido.fetch(...)) and then open it with hmi_map = sunpy.map.Map(files[0]), all the things fix_hmi_meta should fix are still in it, like hmi_map.meta['cunit2'] = 'Sine Latitude'. Am I doing it wrong?

@nabobalis
Copy link
Contributor Author

nabobalis commented Sep 17, 2024

But I'm a bit confused regarding fix_hmi_meta. You state that it's deprecated because this is now fixed within sunpy when you load a HMI file. But if I download a HMI file (with files = Fido.fetch(...)) and then open it with hmi_map = sunpy.map.Map(files[0]), all the things fix_hmi_meta should fix are still in it, like hmi_map.meta['cunit2'] = 'Sine Latitude'. Am I doing it wrong?

Oh in that case, I was very wrong and we should fix that in the map source.

The reason I said that was it was in the notes:

If you have sunpy > 2.1 installed, this function is not needed as sunpy
will automatically make these fixes.

But clearly I didn't check.

@jgieseler
Copy link
Member

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

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

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

@nabobalis
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add towncrier
3 participants