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

Error handling and CLI argument validation #110

Merged
merged 10 commits into from
Apr 30, 2024
Merged

Error handling and CLI argument validation #110

merged 10 commits into from
Apr 30, 2024

Conversation

astronomerritt
Copy link
Collaborator

@astronomerritt astronomerritt commented Apr 24, 2024

Fixes #85.

  • Wrote a class, AdlerCLIArguments, which takes the argparse.Namespace object as input and stores and validates all of the command-line arguments.
  • Currently it performs some basic checks to make sure the command-line input is sensible. This can easily be added to as we invent more command-line arguments or come up with more interesting ways the user can provide silly input.
  • Wrote unit test and docstrings for the class.
  • Rewrote adler.py to fit in with the new code.

Fixes #34.

  • If the RSP has not populated a particular field in the table (for example, DP0.3 doesn't have any values for the u or y filter in DIASource/SSSource), we now check for this, populate that dataclass attribute with a NaN, and print a warning to the terminal.
  • The original warning has been suppressed because it's not as useful/informative as the one I wrote.
  • Docstrings and unit test for additional function I wrote to check if a value has populated.

Minor changes:

  • adler.py was failing on the RSP because it was using the old formulation of SSObject. Fixed it.
  • I tidied up the constructors on MPCORB, Observations and SSObject to use dictionaries. They're nicer now. This is the kind of thing most people don't care about but I find immensely satisfying.
  • The long name for the -s CL argument is now --ssObjectId to fit the rest of the code.
  • The attribute Observations.midpointMjdTai is now Observations.midPointMjdTai to match the column heading.

Review Checklist for Source Code Changes

  • Does pip install still work?
  • Have you written a unit test for any new functions?
  • Do all the units tests run successfully?
  • Does adler run successfully on a test set of input files/databases?
  • Have you used black on the files you have updated to confirm python programming style guide enforcement?

@astronomerritt astronomerritt marked this pull request as ready for review April 24, 2024 16:54
@astronomerritt astronomerritt requested a review from jrob93 April 24, 2024 16:54
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.

Hey I just tried this out on the RSP, looks pretty good to me. My only suggestion would be to add a bit more info in adler --help call --filter_list, it just took me a minute to figure out the formatting.

@jrob93
Copy link
Collaborator

jrob93 commented Apr 26, 2024

Also, this is more of a new feature request: should we have a CLI flag for input options other than construct_from_RSP, e.g. construct_from_data_table, construct_from_SQL?

@mschwamb mschwamb merged commit 5c25e67 into main Apr 30, 2024
10 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
3 participants