Horizons

[szilac]

I added the dataclasses to the local_dataclasses.py file to make it easier to call for other interfaces. I included the default_observer_location option, but if the user wants to, he can change it. I uploaded unit tests for the horizons interface.

[github-actions]

Before [0de6be8]	After [4b87387]	Ratio	Benchmark (Parameter)
416	1.42k	3.42	benchmarks.mem_list
2.84±1s	2.79±1s	0.98	benchmarks.time_computation

Click here to view all benchmarks.

[szilac]

Now, it is ready. I think.

[mschwamb]

@mkelley @akoumjian you're on deck to review this with assist from @jrob93

[jrob93]

Hey, I've had a look through the PR and played around with the code on my local machine. I think this looks good overall, I just have a few comments for consideration:
query_single_range

• should it also accept Horizons kwargs (e.g. id_type, closest apparition etc) as a dict or similar? For example, the user could pass params_dict to query_single_range and then

params_dict = {"id_type":"smallbody"}
obj = Horizons(
id=query.target,
location=self.observer_location,
epochs={"start": query.start.iso, "stop": query.end.iso, "step": query.step},
**params_dict
)

query_ephemeris_from_csv

• this fails if the "data" directory is not in cwd to save the ecsv files. Should this dir be created automatically or be able to be set by the user?
• the function saves results to ecsv, would it be a good idea to make it more consistent with query_single_range by returning a QueryResult object (or list of QueryResults)? I can see the value of saving results to ecsv for very big queries but maybe make saving to file optional? This may end up taking up a user's storage if the saved results are not cleared out.
• At some point perhaps there should be a function to create a QueryResult object from an ecsv file.

Hope this is helpful!

[akoumjian]

Looks good. Does this have a specific ticket in Issues that is it addressing?

I would recommend a couple changes first.

1. Since this is all part of the Ephemeris Service (more like a module or client), I would put this inside a subpackage forcedphot.ephemeris to keep it contained there.
2. I think if this tool is definitely going to support multiple ephemeris clients, now is the time to write the generic EphemerisClient interface, which in turn calls the horizons interface. That should either live in src/forcedphot/ephemeris/__init__.py or in src/forcedphot/ephemeris/client.py, or something similar.
3. See my notes on what level we serialize the data to ECSV. In general, I would only pass files around and references to file paths as a last resort when data is getting too big for memory or we need to send a lot of data over an API request and it's easier to reference a URL to a file. Not sure how @talister feels about that.

[akoumjian]

I know the requirements are the outputs are provided in ECSV. I think it will be easier and make more sense to hold off on writing a file until we need to communicate directly to the client (e.g. over an https request).

I would suggest horizons client, the skybot client, etc. return the EphemerisData object. Then either A) EphemerisService high-level client produces a file or what I like more is B) the Object Detection Controller formatting outputs to return via the API.

[szilac]

Ok. I changed the file structure. The horizons interface has been added to the ephemeris subpackage.
I will add the EphemerisClient at a later time, after at least the Horizons PR has been implemented.
I added a switch to prevent saving ephemeris data to an ecsv file. Currently, both methods (single target, multiple target) return the QueryResult dataclass that contains the ephemeris data.