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
Implemented Groundclients in continuation with #2619 #3763
Conversation
Hello @abhijeetmanhas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-02-16 19:34:31 UTC |
A question: should we render the observatory as "Kanzelhoehe", or even more correctly as "Kanzelhöhe", rather than "Kanzelhohe"? One reason that I ask is that Kanzelhoehe is spelled differently in another issue than it is here, and that frustrated my initial attempt at searching. |
I'll point out that if this PR gets merged, we really ought to fix how Kanzelhohe/Kanzelhoehe/Kanzelhöhe files are read in by |
On it . |
There are a lot of doc string changes to be made having glanced over it. |
Would it be possible/easy to split this into one PR per source? That would make it a lot easier to review |
Yes, I can do it, since all are independent, should this pr be closed now?
…On Wed, 5 Feb 2020 at 22:27, David Stansby ***@***.***> wrote:
Would it be possible/easy to split this into one PR per source? That would
make it a *lot* easier to review
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3763?email_source=notifications&email_token=AKI5PGYSK322W37ZGGIZA3TRBLVX5A5CNFSM4KQK5DDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK4FKEA#issuecomment-582505744>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKI5PG5Q5MJEHX247GZMJM3RBLVX5ANCNFSM4KQK5DDA>
.
|
Feel free to leave this PR open if you want, and open other smaller ones that split this one up. |
Ok. But there were multiple individual PRs of each client earlier, and @dpshelio collected and corrected all of then in #2619 . So I thought continuing from this PR should be more easy rather than starting afresh from individual PRs.
But if it makes review difficult, I will split it.
…On 6 Feb 2020 15:31, "David Stansby" ***@***.***> wrote:
Feel free to leave this PR open if you want, and open other smaller ones
that split this one up.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3763?email_source=notifications&email_token=AKI5PG7RKXL7AM4GLSHNOVTRBPNW3A5CNFSM4KQK5DDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK6TRKQ#issuecomment-582826154>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKI5PG6N7VVC6MCUC74BSWLRBPNW3ANCNFSM4KQK5DDA>
.
|
2bb64c1
to
36c1743
Compare
36c1743
to
371828d
Compare
6f5dd07
to
d4fdee7
Compare
@nabobalis @dstansby it fails on
But I'm still getting the same errors, why is it so? |
The servers are broken. |
Yeah, I say split it up into one PR per client and then one PR for what looks like a bug fix for the scrapper? Doing so means the commit history will be erased but I am ok with that. |
That was my only doubt, with commit history being erased . Thanks for
clearing it. Creating separate PRs now.
…On 17 Feb 2020 02:13, "Nabil Freij" ***@***.***> wrote:
Yeah, I say split it up into one PR per client and then one PR for what
looks like a bug fix for the scrapper?
Doing so means the commit history will be erased but I am ok with that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3763?email_source=notifications&email_token=AKI5PG4N2BASYXHR2Q4GHY3RDGQPTA5CNFSM4KQK5DDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL4RUGI#issuecomment-586750489>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKI5PGZ27I4YJEPW5HBT7PDRDGQPTANCNFSM4KQK5DDA>
.
|
@nabobalis @dstansby All 4 splits can be reviewed now, it have passed all the tests except suvi and changelog is also there. |
A comment about all the clients, we should make sure that the VSO hasn't started indexing these data in the meantime. I am pretty sure you can at least get some GONG data from VSO now. |
The VSO currently has all the GONG data, available under the provider NSO. We also have KSO data. The BBSO data is currently offline under provider = HANET. We'll let you know when it's restored. I'll have to check on VSM and FARSIDE. |
The VSO has VSM data as well under provider = NSO and source = SOLIS. The Farside GONG data is currently not available thru the VSO, but is available directly from NSO's farside website. |
So except kanzelhohe, should PRs about bbso, gong, farside and vsm should be closed? |
We should double check that the clients and the VSO are returning exactly the same data products, but if they are we should close them. In my opinion there is no good reason to implement code we have to maintain if the VSO are already doing it for us? |
This is mine too. I rather not keep duplicate code if the VSO already handles these sources for us. |
I don't found data from Also, can't find There is bbso on VSO, but don't know how to get data from it, as I applied a filter time interval or two years and it displayed something like "no records found" . I think we have to first check whether the search queries on VSO can search the correct data (same as these clients, if implementation is correct, or verify from official observatories' website) and then is data on VSO is subset of actual data? It is in the case of |
If I remember right... I had worked on all these clients mostly because they were providing data in real time whereas VSO was not (needed for SolarMonitor). This may have changed. |
@ejm4567 I feel we can't have access to |
@ejm4567 does VSO covers data from |
Description
It is continuation of PR #2619, it implements kanzelhohe, gong, farside, vsm and bbso clients.