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

Add support for Map to load from a generator #7024

Merged
merged 27 commits into from Apr 12, 2024
Merged

Conversation

Jett-Code
Copy link
Contributor

PR Description

Fixes #7022

@nabobalis
Copy link
Contributor

nabobalis commented May 25, 2023

Looks like this change has failed on the CI, this will need debugging.

This will need a unit test as well to check that pathlib.glob works.

@nabobalis nabobalis added Minor Change PR only needs one approval to merge backport 5.0 on-merge: backport to 5.0 labels May 25, 2023
@Jett-Code
Copy link
Contributor Author

Looks like this change has failed on the CI, this will need debugging.

This will need a unit test as well to check that pathlib.glob works.

Could you please guide me on how to write unit tests for this?

@nabobalis
Copy link
Contributor

Looks like this change has failed on the CI, this will need debugging.
This will need a unit test as well to check that pathlib.glob works.

Could you please guide me on how to write unit tests for this?

You will want to add a unit test to https://github.com/sunpy/sunpy/blob/main/sunpy/map/tests/test_map_factory.py

that basically runs some version of this code:

import pathlib
import sunpy.map
sunpy.map.Map(pathlib.Path.home().glob('*.fits'))

with the path correctly changed to use some test data.
To ensure we support this feature.

You will want to add extra unit tests to https://github.com/sunpy/sunpy/blob/main/sunpy/util/tests/test_util.py to ensure that we skip over strings correctly.

Hopefully that helps.

@Jett-Code Jett-Code requested a review from a team as a code owner May 25, 2023 21:19
@Jett-Code
Copy link
Contributor Author

You will want to add extra unit tests to https://github.com/sunpy/sunpy/blob/main/sunpy/util/tests/test_util.py to ensure that we skip over strings correctly.

There are already two unit test cases for string, list, and tuples. Do you want me to add extra on top of that?
lst = ['a', 'b', [], (['c', 'd']), tuple(), ['e']]

@nabobalis
Copy link
Contributor

There are already two unit test cases for string, list, and tuples. Do you want me to add extra on top of that? lst = ['a', 'b', [], (['c', 'd']), tuple(), ['e']]

If the tests for those are already there, then its ok. But what if you just pass in a string?

The other unit tests are failing with these changes, we need to debug this.

@Jett-Code Jett-Code closed this May 27, 2023
@Jett-Code Jett-Code reopened this May 27, 2023
@nabobalis nabobalis marked this pull request as draft May 30, 2023 11:15
@nabobalis nabobalis changed the title changed isinstance function Add support for Map to load from a generator May 30, 2023
@dstansby dstansby added No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) and removed backport 5.0 on-merge: backport to 5.0 labels May 30, 2023
sunpy/util/util.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Stale The bot will close this PR after 6 months label Dec 18, 2023
@nabobalis nabobalis added the Needs Adoption PRs that were abandoned but should be picked up again and merged in label Dec 18, 2023
@github-actions github-actions bot removed the Stale The bot will close this PR after 6 months label Dec 19, 2023
@nabobalis

This comment was marked as outdated.

@Jett-Code

This comment was marked as outdated.

@nabobalis nabobalis removed Needs Adoption PRs that were abandoned but should be picked up again and merged in Minor Change PR only needs one approval to merge labels Mar 22, 2024
@nabobalis

This comment was marked as outdated.

@nabobalis nabobalis added the Needs Review Needs reviews before merge label Mar 22, 2024
@nabobalis nabobalis marked this pull request as ready for review March 22, 2024 18:20
@nabobalis nabobalis added the map Affects the map submodule label Mar 22, 2024
@nabobalis
Copy link
Contributor

The last set of pushes broke the pull request, can you revert it or fix it please?

@Jett-Code
Copy link
Contributor Author

The last set of pushes broke the pull request, can you revert it or fix it please?

Is it possible to delete the last commit?

@nabobalis
Copy link
Contributor

The last set of pushes broke the pull request, can you revert it or fix it please?

Is it possible to delete the last commit?

Do we not need the changes in that commit?

@nabobalis
Copy link
Contributor

The entire commit history is a bit strange right now. Seems to be repeated?

@nabobalis
Copy link
Contributor

I pushed from an old branch I had locally. Not sure if it was up to date tho.

@Jett-Code
Copy link
Contributor Author

I pushed from an old branch I had locally. Not sure if it was up to date tho.

Thanks, let me check

@Jett-Code
Copy link
Contributor Author

I guess everything is working fine now

@Jett-Code
Copy link
Contributor Author

Is there anything else to work on regarding this pr?

@nabobalis
Copy link
Contributor

Is there anything else to work on regarding this pr?

No, it needs reviews from the other mainainters before a merge.

sunpy/util/tests/test_util.py Outdated Show resolved Hide resolved
@nabobalis nabobalis added Merge When CI Passes Hit that merge button when it's all green! and removed Needs Review Needs reviews before merge labels Apr 12, 2024
@nabobalis nabobalis merged commit a7bb396 into sunpy:main Apr 12, 2024
22 of 27 checks passed
@nabobalis
Copy link
Contributor

Thanks for the PR @Jett-Code, sorry it took so long on our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule Merge When CI Passes Hit that merge button when it's all green! No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map factory does not support pathlib.Path.glob
6 participants