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

Update EISMap #50

Merged
merged 8 commits into from
Nov 10, 2022
Merged

Conversation

wtbarnes
Copy link
Contributor

@wtbarnes wtbarnes commented Oct 26, 2022

The first iteration of EISMap was written against an older version of sunpy. This refactors the EISMap source to take advantage of features in sunpy >= 3.1. It also avoids making modifications to the metadata in place and instead overrides properties of GenericMap to accomplish the same task.

Additionally, this pins to sunpy >= 4.0 as releases older than this are now unsupported (e.g. there will be no bugfix releases for versions older than 4.0).

A few other minor fixes:

  • The measurement property now just returns the thing being measured (e.g. intensity, velocity)
  • The nickname property has been overridden to 'Hinode EIS <line_id> '
  • Make pinning for sphinx less restrict. This is needed as sphinx < 4.2 is not compatible with Python 3.10 (see Sphinx doesn't support python 3.10 sphinx-doc/sphinx#9562

This PR should be squashed before merging

@PaulJWright
Copy link

PaulJWright commented Nov 8, 2022

This is a significant change which I believe removes support for python 3.7, and sunpy < 4.0.

If this is going to be merged, I would prefer this was done, tested, and documented before the completion of the JOSS review. This may however be out of scope. I will consult with @mbobra and @nabobalis in the review thread: openjournals/joss-reviews#4914

- python-version: 3.8
- python-version: 3.9
- python-version: '3.10'

Choose a reason for hiding this comment

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

Do you want to also try 3.11?

Choose a reason for hiding this comment

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

This should also be updated in the setup.cfg.

@MJWeberg
Copy link
Collaborator

Thanks @wtbarnes, for updating this! Since we got burned by dependency updates in the past, I did some extra testing in an isolated environment (not doubting your code, just trying to be extra careful in case something else in the dependency chain broke).

Everything looks great! I shall go ahead and merge it.

@nabobalis, I believe I saw something recently about NDCube still testing Python 3.11 support. Since this is one of our main dependencies, would you recommend we start testing 3.11 ourselves, or wait until the next NDCube update?

@MJWeberg MJWeberg merged commit 7b522e1 into USNavalResearchLaboratory:main Nov 10, 2022
@PaulJWright
Copy link

@MJWeberg before this is merged you may want to check that it updates the docs etc reflecting the versions

@nabobalis
Copy link

@nabobalis, I believe I saw something recently about NDCube still testing Python 3.11 support. Since this is one of our main dependencies, would you recommend we start testing 3.11 ourselves, or wait until the next NDCube update?

It was just me testing that ndcube works under 3.11. From what I recall, there was no problems for ndcube.

Your issue for 3.11 might be that there are no wheels for h5py which means you need to compile them on the CI.

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.

None yet

4 participants