Ephemeris service #11

szilac · 2024-07-23T13:02:32Z

Change Description

The branch contains the full ephemeris service component. ephemeris_client.py controls the various interfaces. The outputs are uniformized. Each QueryResult returns a dataclass object or a QueryResult list.

Fixed issues:
--> closes #6
--> closes #7
--> closes #8
--> closes #10

New Feature Checklist

Now it is a complete ephemeris service. One can call the ephemeris_client.py with args and control the different interfaces, like JPL Horizons, Miriade, custom data loader
All of the relevant methods contains save_data options (default: False)

Documentation Change Checklist

Updated most of the docstring

modified: src/forcedphot/horizons_interface.py

…into horizons

deleted: src/forcedphot/data/targets.csv modified: src/forcedphot/local_dataclasses.py modified: tests/forcedphot/conftest.py

modified: src/forcedphot/horizons_interface.py modified: tests/forcedphot/test_horizons_interface.py

src/forcedphot/data/

modified: src/forcedphot/horizons_interface.py modified: src/forcedphot/local_dataclasses.py modified: tests/forcedphot/conftest.py deleted: tests/forcedphot/test_example_module.py modified: tests/forcedphot/test_horizons_interface.py

modified: local_dataclasses.py

…eryResult dataclass now. modified: src/forcedphot/horizons_interface.py

modified: src/forcedphot/ephemeris/horizons_interface.py modified: tests/forcedphot/ephemeris/test_ephemeris_client.py

mschwamb · 2024-07-23T17:35:41Z

I think data directory should go under tests

mschwamb · 2024-07-23T17:39:03Z

Discussion from the telecon is that ecsv handling is going to be updated

mkelley

Some initial comments. I will review more later.

src/forcedphot/ephemeris/local_dataclasses.py

src/forcedphot/ephemeris/horizons_interface.py

src/forcedphot/ephemeris/data_loader.py

src/forcedphot/ephemeris/horizons_interface.py

akoumjian

I agree with @mkelley about being explicit regarding time scale on every Time initialization, as well as documenting what scale we use in serialization since the scale does not survive the serialization process. Non-blocking suggestions from my side.

akoumjian · 2024-07-30T16:06:09Z

src/forcedphot/ephemeris/data_loader.py

+if __name__ == "__main__":
+    # Example usage
+    # file_path = "./Ceres_2024-01-01_00-00-00.000_2025-12-31_23-59-00.000.ecsv"
+    # try:
+    #     ephemeris_data = DataLoader.load_ephemeris_from_ecsv(file_path)
+    # except Exception as e:
+    #     print(f"Error: {str(e)}")
+
+    # Example of loading multiple files
+    file_paths = [
+        "./Ceres_2024-01-01_00-00-00.000_2025-12-31_23-59-00.000.ecsv",
+        "./Encke_2024-01-01_00-00-00.000_2024-06-30_23-59-00.000.ecsv",
+    ]
+    try:
+        ephemeris_list = DataLoader.load_multiple_ephemeris_files(file_paths)
+        print(f"Loaded {len(ephemeris_list)} ephemeris files.")
+    except Exception as e:
+        print(f"Error: {str(e)}")


This kind of looks like a unit test.

Yes, a little bit. I was just verifying that the module was working.

Since we have it, why not put these files into the test data directory and make it a unit test?

akoumjian · 2024-07-30T16:08:31Z

src/forcedphot/ephemeris/ephemeris_client.py

+
+    def query_single(
+        self,
+        service: str,


I would personally prefer service as a keyword argument. That way the client can use its own logic to intelligently choose which service to use, but it can be forced to use a particular downstream service.

I have changed the service part. Now the default is horizons and can be overridden with --service keyword.

akoumjian · 2024-07-30T16:13:32Z

src/forcedphot/ephemeris/ephemeris_client.py

+            List of query results.
+        """
+        if service.lower() == "horizons":
+            return HorizonsInterface.query_ephemeris_from_csv(


Any reason the _from_csv can't live on the generic client? If the CSV input format is identical, I would probably make it the job of the generic client to handle parsing the csv and loading it into the format for the query.

I can change it that way if you think it would be better, but I prefer to have all the functions for a particular service in one place.
However, it is true that the code fragment is the same in both services...

I think it would be cleaner to have one place where the csv is parsed and that the pluggable backends take as input the pre-parsed data, but not a blocker.

renamed: src/forcedphot/ephemeris/local_dataclasses.py -> src/forcedphot/ephemeris/data_model.py modified: src/forcedphot/ephemeris/ephemeris_client.py modified: src/forcedphot/ephemeris/horizons_interface.py modified: src/forcedphot/ephemeris/miriade_interface.py renamed: src/forcedphot/data/targets.csv -> tests/forcedphot/ephemeris/data/targets.csv modified: tests/forcedphot/ephemeris/test_data_loader.py modified: tests/forcedphot/ephemeris/test_ephemeris_client.py modified: tests/forcedphot/ephemeris/test_horizons_interface.py modified: tests/forcedphot/ephemeris/test_miriade_interface.py

modified: src/forcedphot/ephemeris/horizons_interface.py modified: src/forcedphot/ephemeris/miriade_interface.py modified: tests/forcedphot/ephemeris/test_ephemeris_client.py modified: tests/forcedphot/ephemeris/test_horizons_interface.py

mkelley

Thanks, I learned a couple tricks reviewing your code 👍🏻

src/forcedphot/ephemeris/miriade_interface.py

modified: src/forcedphot/ephemeris/data_model.py modified: src/forcedphot/ephemeris/horizons_interface.py modified: src/forcedphot/ephemeris/miriade_interface.py modified: tests/forcedphot/ephemeris/test_data_loader.py modified: tests/forcedphot/ephemeris/test_horizons_interface.py modified: tests/forcedphot/ephemeris/test_miriade_interface.py

modified: src/forcedphot/ephemeris/miriade_interface.py modified: tests/forcedphot/ephemeris/test_horizons_interface.py modified: tests/forcedphot/ephemeris/test_miriade_interface.py

mschwamb · 2024-08-20T16:38:48Z

@akoumjian Are you happy to approve?

mschwamb · 2024-08-26T10:06:46Z

@talister can you review if @akoumjian by your morning Tuesday? It would be good to get this part into the repo

talister

This looks good to me. I've highlighted where you may want to refactor the logging into a base logging configuration file later once more modules are written but this is OK now.
Similarly the argument parser for the EphemerisClient could do with more examples in the help of the specific format of some of the entries or embedding the default value into the help text, but since this client won't be used by end users directly, it's more appropriate that this is applied to the user-facing part of the system.

talister · 2024-08-26T16:48:42Z

src/forcedphot/ephemeris/data_loader.py

+            Load ephemeris data from multiple ECSV files and return them as a list of EphemerisData objects.
+    """
+
+    # Set up logging


This is OK for now but we may want consider splitting the base logging configuration into a separate file (e.g. this brief demo) more modules are written

akoumjian

A couple of suggestions remain, but none are blocking.

akoumjian · 2024-08-26T17:22:04Z

src/forcedphot/ephemeris/data_loader.py

+if __name__ == "__main__":
+    # Example usage
+    # file_path = "./Ceres_2024-01-01_00-00-00.000_2025-12-31_23-59-00.000.ecsv"
+    # try:
+    #     ephemeris_data = DataLoader.load_ephemeris_from_ecsv(file_path)
+    # except Exception as e:
+    #     print(f"Error: {str(e)}")
+
+    # Example of loading multiple files
+    file_paths = [
+        "./Ceres_2024-01-01_00-00-00.000_2025-12-31_23-59-00.000.ecsv",
+        "./Encke_2024-01-01_00-00-00.000_2024-06-30_23-59-00.000.ecsv",
+    ]
+    try:
+        ephemeris_list = DataLoader.load_multiple_ephemeris_files(file_paths)
+        print(f"Loaded {len(ephemeris_list)} ephemeris files.")
+    except Exception as e:
+        print(f"Error: {str(e)}")


Since we have it, why not put these files into the test data directory and make it a unit test?

akoumjian · 2024-08-26T17:24:18Z

src/forcedphot/ephemeris/ephemeris_client.py

+            List of query results.
+        """
+        if service.lower() == "horizons":
+            return HorizonsInterface.query_ephemeris_from_csv(


I think it would be cleaner to have one place where the csv is parsed and that the pluggable backends take as input the pre-parsed data, but not a blocker.

szilac added 30 commits July 9, 2024 12:48

reupload the horizons interface with unit test

60ad788

add target.csv

f26505b

deleted: data/targets.csv

b3af98c

modified: src/forcedphot/horizons_interface.py

Update requirements.txt

d79837a

modified: pyproject.toml

ccc1418

Merge branch 'horizons' of https://github.com/lsst-sssc/ssoforcedphot …

0a3d932

…into horizons

Update pyproject.toml

56ec7ce

Update pyproject.toml

e7451f0

modified: src/forcedphot/__init__.py

eca5854

modified: tests/forcedphot/test_horizons_interface.py

b524aa8

modified: tests/forcedphot/test_horizons_interface.py

f98a7d0

modified: pyproject.toml

01bc6ed

deleted: src/forcedphot/data/targets.csv modified: src/forcedphot/local_dataclasses.py modified: tests/forcedphot/conftest.py

modified: src/forcedphot/__init__.py

a0394ff

modified: src/forcedphot/horizons_interface.py modified: tests/forcedphot/test_horizons_interface.py

modified: tests/forcedphot/conftest.py

5fb5d18

modified: tests/forcedphot/conftest.py

8e23398

modified: src/forcedphot/local_dataclasses.py

b0bc4d7

modified: src/forcedphot/local_dataclasses.py

96de6d2

modified: src/forcedphot/horizons_interface.py

1c5485e

src/forcedphot/data/

new file: src/forcedphot/data/targets.csv

2f61d83

deleted: src/forcedphot/example_module.py

d8960e4

modified: src/forcedphot/horizons_interface.py modified: src/forcedphot/local_dataclasses.py modified: tests/forcedphot/conftest.py deleted: tests/forcedphot/test_example_module.py modified: tests/forcedphot/test_horizons_interface.py

Update horizons_interface.py

6d7f529

Update test_horizons_interface.py

e38b24c

Update horizons_interface.py

a22ea06

Update local_dataclasses.py

f7a9c17

Update local_dataclasses.py

b05e286

modified: horizons_interface.py

6f8ced8

modified: local_dataclasses.py

modified: ../../tests/forcedphot/test_horizons_interface.py

ead6bb1

I rewrote the horizons_interface outputs. The standard output is a Qu…

540baf0

…eryResult dataclass now. modified: src/forcedphot/horizons_interface.py

modified: src/forcedphot/horizons_interface.py

ee46ac4

modified: src/forcedphot/horizons_interface.py

5b9aa4a

modified: src/forcedphot/ephemeris/ephemeris_client.py

08910d7

modified: src/forcedphot/ephemeris/horizons_interface.py modified: tests/forcedphot/ephemeris/test_ephemeris_client.py

szilac marked this pull request as ready for review July 23, 2024 16:49

mschwamb requested review from mkelley and akoumjian July 23, 2024 17:14

mkelley reviewed Jul 24, 2024

View reviewed changes

akoumjian previously approved these changes Jul 30, 2024

View reviewed changes

szilac dismissed akoumjian’s stale review via ee1039c August 2, 2024 12:55

mkelley requested changes Aug 7, 2024

View reviewed changes

src/forcedphot/ephemeris/miriade_interface.py Outdated Show resolved Hide resolved

src/forcedphot/ephemeris/miriade_interface.py Outdated Show resolved Hide resolved

szilac added 6 commits August 13, 2024 16:25

modified: src/forcedphot/ephemeris/horizons_interface.py

1a6f0bd

modified: src/forcedphot/ephemeris/miriade_interface.py modified: tests/forcedphot/ephemeris/test_horizons_interface.py modified: tests/forcedphot/ephemeris/test_miriade_interface.py

modified: src/forcedphot/ephemeris/horizons_interface.py

5e58792

modified: src/forcedphot/ephemeris/horizons_interface.py

6074254

modified: src/forcedphot/ephemeris/ephemeris_client.py

5089ec3

modified: src/forcedphot/ephemeris/ephemeris_client.py

0705ba0

mschwamb requested review from mkelley and akoumjian August 13, 2024 17:20

mkelley approved these changes Aug 16, 2024

View reviewed changes

mschwamb requested a review from talister August 26, 2024 10:02

talister approved these changes Aug 26, 2024

View reviewed changes

akoumjian approved these changes Aug 26, 2024

View reviewed changes

This was referenced Aug 26, 2024

move ephemeris generation files into a directory and make a unit test #15

Closed

one place were csvs are parsed #17

Open

initalize logging #18

Open

mschwamb merged commit b19efc8 into main Aug 27, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ephemeris service #11

Ephemeris service #11

szilac commented Jul 23, 2024

mschwamb commented Jul 23, 2024

mschwamb commented Jul 23, 2024

mkelley left a comment

akoumjian left a comment

akoumjian Jul 30, 2024

szilac Aug 13, 2024

akoumjian Aug 26, 2024

akoumjian Jul 30, 2024

szilac Aug 13, 2024

akoumjian Jul 30, 2024

szilac Aug 13, 2024

akoumjian Aug 26, 2024

mkelley left a comment

mschwamb commented Aug 20, 2024

mschwamb commented Aug 26, 2024

talister left a comment

talister Aug 26, 2024

akoumjian left a comment

akoumjian Aug 26, 2024

akoumjian Aug 26, 2024

Ephemeris service #11

Ephemeris service #11

Conversation

szilac commented Jul 23, 2024

Change Description

New Feature Checklist

Documentation Change Checklist

mschwamb commented Jul 23, 2024

mschwamb commented Jul 23, 2024

mkelley left a comment

Choose a reason for hiding this comment

akoumjian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkelley left a comment

Choose a reason for hiding this comment

mschwamb commented Aug 20, 2024

mschwamb commented Aug 26, 2024

talister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoumjian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment