New Horizons API #32

LuEdRaMo · 2024-01-13T16:02:51Z

In order to address #31, this PR translates functions smb_spk, smb_spk_ele and vec_tbl to the new Horizons API.

LuEdRaMo · 2024-01-13T16:12:54Z

@PerezHz what are your thoughts on this PR? Also, I have a couple of questions:

Currently, I separate the original functions from the new ones via a ::Val{API} argument but, Is it worth keeping the telnet methods, since those require installing telnet externally while the new ones do not?
Should I go ahead and implement other JPL APIs such as these?

PerezHz · 2024-01-13T17:04:11Z

Hey @LuEdRaMo! Many thanks for this PR! I'll have a look to the details of this PR as soon as I can.

I would actually be in favor of scrapping the telnet methods (thus eliminating an external dependency in favor of a safer and more modern data-fetching API); ie I'm in favor of switching to the new HTTP API completely. Nostalgia-driven users won't mind using an older HORIZONS.jl version 🙃.

Regarding your second question, sure go ahead!

PerezHz · 2024-01-13T17:06:12Z

Just a small caveat: we should add a warning to the users explaining that we switched APIs from telnet to HTTP.

LuEdRaMo · 2024-01-22T17:52:31Z

I have a few more comments/questions:

Should we bump the major or the minor version?
Except for horizons(), the other functions have been adapted to the new API.
The only function that I have not adapted yet is vec_tbl_csv, is it worth keeping it? I feel that it could belong to a DataFrames extension.
I still have to update README.md.
I've written new tests.
So far, I've implemented 3 of the JPL APIs (Horizons, SBDB and Radar Astrometry), I think the rest can be the subject of future PRs.

PerezHz · 2024-01-29T22:00:31Z

Should we bump the major or the minor version?

Before 1, versions 0.x are considered breaking among themselves, so bumping the minor version should enough in this case.

Except for horizons(), the other functions have been adapted to the new API.

Okay, eventually the telnet method to connect to Horizons will be removed by JPL, but let's keep it for the time being.

The only function that I have not adapted yet is vec_tbl_csv, is it worth keeping it? I feel that it could belong to a DataFrames extension.

I agree that vec_tbl_csv is not essential and perhaps a bit redundant; feel free to deprecate it.

I still have to update README.md.

So far it has worked as the documentation of the package, so on this point I do think it's worth to update together with this PR.

I've written new tests.

Many thanks for adding those!

So far, I've implemented 3 of the JPL APIs (Horizons, SBDB and Radar Astrometry), I think the rest can be the subject of future PRs.

I think that's more than enough for what the package is already covering; I agree we can leave the rest of the JPL APIs for future PRs, I'd suggest to just open an issue here regarding that point.

PerezHz · 2024-01-29T22:01:28Z

Btw, julia 1.0 tests are failing, can you update the CI.yml to test julia 1.6, 1 and nightly?

Project.toml

PerezHz · 2024-01-30T12:41:58Z

test/runtests.jl

+    @test isa(apophis_1["data"], Vector{Any})
+    @test isa(apophis_1["count"], String)
+    @test length(apophis_1["data"]) == parse(Int, apophis_1["count"])
+


I think around here we can add some tests to check the output, e.g.:

Suggested change

@test apophis_1["fields"] == ["des", "epoch", "value", "sigma", "units", "freq", "rcvr", "xmit", "bp", "observer"]

@test all(first.(apophis_1["data"]) .== "99942")

@test apophis_1["signature"]["source"] == "NASA/JPL Small-Body Radar Astrometry API"

@test VersionNumber(apophis_1["signature"]["version"]) ≥ v"1"

(EDIT: added @test to suggestions above)

PerezHz · 2024-01-30T13:03:16Z

test/runtests.jl

+    @test isa(apophis_2["data"], Vector{Any})
+    @test isa(apophis_2["count"], String)
+    @test length(apophis_2["data"]) == parse(Int, apophis_2["count"])
+


@test apophis_2["fields"][end] == "observer"

LuEdRaMo · 2024-01-30T14:58:34Z

In the last commits I've addressed all your comments @PerezHz. I think this PR is ready for review.

PerezHz

I think this is essentially ready for merging, there's just a couple of issues with the tests that we should address first, otherwise, LGTM! Many thanks again @LuEdRaMo!

PerezHz · 2024-01-30T16:38:43Z

test/runtests.jl

+    x = readlines(local_file)
+    y = split(apophisvt, "\n")[1:end-1]
+
+    @test x == y


Tests are failing in 1.6 and 1.10, I'd suggest to change this test to something like this:

Suggested change

@test x == y

@test length(x) == length(y)

lx = length(x)

inds = setdiff(1:lx,[2,35]) # avoid lines 2 and 35, since they have time of retrieval

@test x[inds] == y[inds]

(edit: above I meant julia 1.6; wrote 1.9 instead)

PerezHz · 2024-01-30T18:15:08Z

@LuEdRaMo Just left one last comment. Let me know when this is ready, and then we should be good to go!

PerezHz · 2024-01-30T18:17:46Z

Deleted my last comment, the branching was swapped; will update my suggestion in a bit 😅

src/common.jl

PerezHz · 2024-01-30T19:21:01Z

I went ahead and committed the suggestion; if we get the green lights I'll merge, thanks a lot!

LuEdRaMo · 2024-01-30T19:43:20Z

I went ahead and committed the suggestion; if we get the green lights I'll merge, thanks a lot!

Thank you @PerezHz and sorry for the delay. On my side everything looks fine...

I'm excited about the applications of this PR in NEOs.

PerezHz · 2024-01-30T20:30:04Z

Thank you @PerezHz and sorry for the delay. On my side everything looks fine...

No worries, and great catch on the breaking changes warning!

I'm excited about the applications of this PR in NEOs.

Indeed, me too! 😄

First draft

1699243

LuEdRaMo changed the title ~~New Horizons API~~ WIP: New Horizons API Jan 13, 2024

New APIs + new tests

b0f1ffb

PerezHz reviewed Jan 29, 2024

View reviewed changes

Project.toml Show resolved Hide resolved

PerezHz reviewed Jan 30, 2024

View reviewed changes

LuEdRaMo added 3 commits January 30, 2024 07:50

Suggestions by @PerezHz

674a737

Update README.md

3a4e7f8

Minor fixes

0485329

LuEdRaMo marked this pull request as ready for review January 30, 2024 14:56

PerezHz reviewed Jan 30, 2024

View reviewed changes

Fix vec_tbl tests

19219b7

PerezHz reviewed Jan 30, 2024

View reviewed changes

src/common.jl Outdated Show resolved Hide resolved

Update common.jl

8004462

LuEdRaMo added 2 commits January 30, 2024 14:20

Breaking change warning

24591f1

Minor fix

43e933f

PerezHz merged commit 8f61817 into PerezHz:master Jan 30, 2024
6 checks passed

PerezHz mentioned this pull request Jan 30, 2024

[WIP] Fix appveyor ci #7

Closed

PerezHz changed the title ~~WIP: New Horizons API~~ New Horizons API Jan 30, 2024

This was referenced Jan 30, 2024

Add support for new HORIZONS APIs #31

Closed

Implement the rest of JPL APIs #34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Horizons API #32

New Horizons API #32

LuEdRaMo commented Jan 13, 2024

LuEdRaMo commented Jan 13, 2024 •

edited

Loading

PerezHz commented Jan 13, 2024 •

edited

Loading

PerezHz commented Jan 13, 2024

LuEdRaMo commented Jan 22, 2024

PerezHz commented Jan 29, 2024

PerezHz commented Jan 29, 2024

PerezHz Jan 30, 2024 •

edited

Loading

PerezHz Jan 30, 2024

PerezHz Jan 30, 2024

LuEdRaMo commented Jan 30, 2024

PerezHz left a comment

PerezHz Jan 30, 2024 •

edited

Loading

PerezHz Jan 30, 2024

PerezHz commented Jan 30, 2024

PerezHz commented Jan 30, 2024

PerezHz commented Jan 30, 2024

LuEdRaMo commented Jan 30, 2024 •

edited

Loading

PerezHz commented Jan 30, 2024

+    @test apophis_1["fields"] == ["des", "epoch", "value", "sigma", "units", "freq", "rcvr", "xmit", "bp", "observer"]
+    @test all(first.(apophis_1["data"]) .== "99942")
+    @test apophis_1["signature"]["source"] == "NASA/JPL Small-Body Radar Astrometry API"
+    @test VersionNumber(apophis_1["signature"]["version"]) ≥ v"1"

-    @test x == y
+    @test length(x) == length(y)
+    lx = length(x)
+    inds = setdiff(1:lx,[2,35]) # avoid lines 2 and 35, since they have time of retrieval
+    @test x[inds] == y[inds]

New Horizons API #32

New Horizons API #32

Conversation

LuEdRaMo commented Jan 13, 2024

LuEdRaMo commented Jan 13, 2024 • edited Loading

PerezHz commented Jan 13, 2024 • edited Loading

PerezHz commented Jan 13, 2024

LuEdRaMo commented Jan 22, 2024

PerezHz commented Jan 29, 2024

PerezHz commented Jan 29, 2024

PerezHz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

PerezHz Jan 30, 2024

Choose a reason for hiding this comment

PerezHz Jan 30, 2024

Choose a reason for hiding this comment

LuEdRaMo commented Jan 30, 2024

PerezHz left a comment

Choose a reason for hiding this comment

PerezHz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

PerezHz Jan 30, 2024

Choose a reason for hiding this comment

PerezHz commented Jan 30, 2024

PerezHz commented Jan 30, 2024

PerezHz commented Jan 30, 2024

LuEdRaMo commented Jan 30, 2024 • edited Loading

PerezHz commented Jan 30, 2024

LuEdRaMo commented Jan 13, 2024 •

edited

Loading

PerezHz commented Jan 13, 2024 •

edited

Loading

PerezHz Jan 30, 2024 •

edited

Loading

PerezHz Jan 30, 2024 •

edited

Loading

LuEdRaMo commented Jan 30, 2024 •

edited

Loading