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

ADAPT Client #7463

Merged
merged 26 commits into from
Jun 10, 2024
Merged

ADAPT Client #7463

merged 26 commits into from
Jun 10, 2024

Conversation

GillySpace27
Copy link
Contributor

PR Description

Adds functionality to download ADAPT maps using FIDO.
This addresses the following issue: #7152

I believe that the core of this code works but I cant get a dev environment running to test things. I believe I'll have to do more work putting the right parts in the right places for sunpy, but I wanted to get this started by uploading what I have.

Copy link
Contributor Author

@GillySpace27 GillySpace27 left a comment

Choose a reason for hiding this comment

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

Ok, I think that the client works now, though there may be some code linting that could be helpful.

Copy link
Contributor

@nabobalis nabobalis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @GillySpace27!

I hope it was ok for me to look over this while it was in draft.

setup.cfg Outdated Show resolved Hide resolved
sunpy/net/__init__.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/attrs/__init__.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/attrs/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/attrs/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@GillySpace27 GillySpace27 left a comment

Choose a reason for hiding this comment

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

I think I did everything except for the test parts, which I'm not experienced with. Please feel free to give this another once-over.

@nabobalis
Copy link
Contributor

Hi @GillySpace27, sorry for not getting back to you sooner, I will go over this soon but I have two suggestions:

  1. Can you add back "sunpy/net/dataretriever/sources/init.py", I think that has to be there.

  2. For the tests, you can avoid having to write the tests from scratch by just copying the file from say one of the goes test files, update the client to use adapt and then tweak the calls to the client with the correct attrs. That should reduce the burden on you and would cover most of the tests we would need for a new client.

sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
sunpy/net/dataretriever/sources/adapt.py Outdated Show resolved Hide resolved
@nabobalis nabobalis added net Affects the net submodule Whats New? Needs a section added to the current Whats New? page. No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Mar 26, 2024
Copy link
Contributor Author

@GillySpace27 GillySpace27 left a comment

Choose a reason for hiding this comment

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

Thanks for the additional comments! I did everything except the tests, which I will definitely get around to ASAP. Let me know if you see anything else!

Copy link
Contributor Author

@GillySpace27 GillySpace27 left a comment

Choose a reason for hiding this comment

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

I just adjusted the main client in response to your comments, but I still haven't touched the tests, which seem daunting. Is there anything else I should do in addition to the tests?

@nabobalis
Copy link
Contributor

I just adjusted the main client in response to your comments, but I still haven't touched the tests, which seem daunting. Is there anything else I should do in addition to the tests?

I can have a look, but for the tests, I could just copy the tests from another client and just modify the names. That should cover most of the tests we would need.

Copy link
Contributor Author

@GillySpace27 GillySpace27 left a comment

Choose a reason for hiding this comment

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

I just adjusted the main client in response to your comments, but I still haven't touched the tests, which seem daunting. Is there anything else I should do in addition to the tests?

I can have a look, but for the tests, I could just copy the tests from another client and just modify the names. That should cover most of the tests we would need.

If you are able to make those changes, that would be rad! looking at the changes you make would probably be useful for me to see. I just copied over the file in my most recent commit.

@nabobalis
Copy link
Contributor

Hi @GillySpace27, I rebased and updated the tests, they should cover what we need.

@nabobalis
Copy link
Contributor

Hi @GillySpace27, I would suggest that instead of trying to merge in the changes locally, that it would be better to run:

git remote update -p
git reset --hard origin/main

This will update your local repo and reset all local changes to point to the current version that is on GitHub.

If you have any questions about git, please just let know!

@nabobalis nabobalis added this to the 6.0.0 milestone May 24, 2024
@nabobalis nabobalis marked this pull request as ready for review May 24, 2024 15:25
@nabobalis nabobalis requested a review from a team as a code owner May 24, 2024 15:25
@nabobalis nabobalis added the Needs Review Needs reviews before merge label May 24, 2024
@nabobalis nabobalis mentioned this pull request May 24, 2024
1 task
@nabobalis
Copy link
Contributor

nabobalis commented May 24, 2024

I forgot how annoying our net subset of unit tests are.

I am still trying to work out what is going on and fix it.
More developer documentation has to be written to account for all these.

@nabobalis nabobalis force-pushed the main branch 2 times, most recently from d0ecee0 to 532e4fd Compare May 24, 2024 18:24
@GillySpace27
Copy link
Contributor Author

Thanks for your help getting this sorted out! Let me know if you need anything from me. :)

@nabobalis nabobalis removed the Needs Review Needs reviews before merge label May 26, 2024
@nabobalis
Copy link
Contributor

Thanks for your help getting this sorted out! Let me know if you need anything from me. :)

Sorry I kind of just took over. There were some parts that I had to recall how it worked for me to patch. I need to increase the developer guide with more details so its more helpful.

If you are ok with the changes, and want to check it still works how you expect it to, that would be great. Then I can merge it.

@GillySpace27
Copy link
Contributor Author

GillySpace27 commented May 31, 2024

So what happens now? I just ran the git commands you suggested and then tested the code by running the example in the docstring, and it seems like it's behaving as expected!

@nabobalis
Copy link
Contributor

Right now, I need to wait for another review before we should merge.

I am hoping that will come as we want to get 6.0 out.

@GillySpace27
Copy link
Contributor Author

Thanks again for your help! I'll mark this off as a backburnered project until I hear back from you all. I hope it makes it in to the release!

@nabobalis
Copy link
Contributor

Online fails are due to HEK and nothing to do with this PR.

@nabobalis nabobalis merged commit 878e2f4 into sunpy:main Jun 10, 2024
23 of 26 checks passed
@nabobalis
Copy link
Contributor

Thanks for the PR @GillySpace27, sorry it took so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) Whats New? Needs a section added to the current Whats New? page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants