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

Object detection controller #19

Merged
merged 24 commits into from
Nov 12, 2024
Merged

Object detection controller #19

merged 24 commits into from
Nov 12, 2024

Conversation

szilac
Copy link
Collaborator

@szilac szilac commented Aug 27, 2024

Change Description

I have created the objection detection controller (odc) which brings together and manages the various subsystems, such as the ephemeris service, the image service, etc.
Currently it only manages the ephemeris service via command line interface.

	new file:   forcedphot/odc.py
	new file:   forcedphot/query_args.txt
	new file:   tests/forcedphot/test_odc.py
	modified:   tests/forcedphot/ephemeris/test_data_loader.py
	modified:   tests/forcedphot/test_odc.py
	modified:   src/forcedphot/ephemeris/horizons_interface.py
	modified:   src/forcedphot/ephemeris/miriade_interface.py
	modified:   src/forcedphot/odc.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
Copy link

github-actions bot commented Aug 27, 2024

Before [578eec8] After [53a6ae0] Ratio Benchmark (Parameter)
3.34±0.7s 1.78±1s ~0.53 benchmarks.time_computation
1.11k 3.71k 3.34 benchmarks.mem_list

Click here to view all benchmarks.

@mkelley
Copy link
Collaborator

mkelley commented Aug 27, 2024

Thank you for your contribution to ssoforcedphot, a LINCC Framework package! 🌌 This checklist is meant to remind pull request reviewers of some common things to look for.

  • Do the proposed changes accomplish the desired goals and meet the acceptance criteria of the related issue?
  • Do the proposed changes follow the coding guidelines?
  • Are tests added/updated as needed? Are methods requiring network access mocked for offline testing?
  • Is there sufficient documentation for the changes?

@talister talister requested a review from ijiraq August 27, 2024 17:12
@talister talister self-assigned this Aug 27, 2024
@talister talister requested a review from carrholt August 27, 2024 17:15
Copy link
Collaborator

@akoumjian akoumjian left a comment

Choose a reason for hiding this comment

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

I would remove all the commented out code, save it to another branch if you want to save it for future use.

src/forcedphot/odc.py Show resolved Hide resolved
@mschwamb
Copy link
Member

mschwamb commented Sep 3, 2024

@carrholt and @ijiraq Would you be able to review by Sep10th?

--ecsv:
Path to ECSV file containing input data.

--csv:
Copy link
Collaborator

@carrholt carrholt Sep 10, 2024

Choose a reason for hiding this comment

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

should give more info on how the csv file should be set up. Based on the test file, it looks like the column headers are named the specific args? But I may be wrong here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the tests/forcedphot/ephemeris/data folder, I added a template_ephemeris.ecsv file that can be used as a template.

Copy link
Collaborator

@carrholt carrholt Sep 10, 2024

Choose a reason for hiding this comment

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

if used for tests, should be moved a data directory in tests or if it's being used as an example, it should probably still go elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are absolutely right, I moved it to the tests/forcedphot/ephemeris/data folder.

service selection, target specification, time range settings, input file
handling, and various service configurations.

Command-line Arguments:
Copy link
Collaborator

@carrholt carrholt Sep 10, 2024

Choose a reason for hiding this comment

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

The filter(s) a user wants to query should be an argument for all but the ephemeris service. It should be able to receive a single filter or list of filters (e.g., [g,r])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this argument was left out. I have included it.
It's not connected to anything at the moment, but as soon as the image service is up and running, it will be used.

Copy link
Collaborator

@carrholt carrholt left a comment

Choose a reason for hiding this comment

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

	modified:   src/forcedphot/ephemeris/horizons_interface.py
	modified:   src/forcedphot/ephemeris/miriade_interface.py
	modified:   src/forcedphot/odc.py
	new file:   tests/forcedphot/ephemeris/data/query_args.txt
	new file:   tests/forcedphot/ephemeris/data/query_args2.txt
	new file:   tests/forcedphot/ephemeris/data/template_ephemeris.ecsv
@jrob93
Copy link
Collaborator

jrob93 commented Oct 25, 2024

I just made some small changes as the conda command in README.md wasn't working for me and there was a typo in odc.py

@mschwamb mschwamb requested a review from jrob93 October 29, 2024 11:48
Copy link
Collaborator

@jrob93 jrob93 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 this all works nicely for the current task of retrieving ephemerides for a given object(s). At some point in the future it would be good to have more documentation on exactly how CLI arguments such as --target-type relates to the Horizons/Miriade query. I had comments on the CLI arguments relating to other aspects of the package (issue #13) but I think this can be merged now and these other details sorted when they are being worked on

@szilac szilac merged commit 8fe0fc1 into main Nov 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants