Generate trajectory #168
Generate trajectory #168
Conversation
tmf97
left a comment
tmf97
left a comment
There was a problem hiding this comment.
Really solid stuff, code-specific comments are in the files.
Higher-level stuff (which is really relevant to not just this PR, but all of our project's software):
- Can we please type hint as much as reasonably possible? It goes a long way (for me, anyways) to understand the inputs, outputs, and functioning of any program.
- Speaking of inputs/outputs, it may simplify our code a lot to pass dataframes or dictionaries between functions (rather than packing and unpacking them from .csv and .json files)
| # Epoch depends on the specific trajectory, but I've left it here for reference -mm2774 | ||
| epoch = "2020-06-27T21:08:03.0212 TDB" |
There was a problem hiding this comment.
Why is this epoch hardcoded? Is it something we can get this time from the input trajectory file?
There was a problem hiding this comment.
This epoch is from a trajectory that Dr. Muhlberger created with Orekit. The Spring 2020 trajectories didn't have references to time in the csv files or in the reports except for the "dt" variable, so I left the older epoch there for now
What's a clean way to deal with this? We could leave it there and have the user define it based on their trajectory. Or we could turn it into an argument. But writing this out in the terminal seems kind of wordy, so I wasn't sure
There was a problem hiding this comment.
Ideally, every trajectory that you get would come with the absolute time built into the position/velocity entries. As mentioned in the other comment, this is fine for development, but mention that this isn't ideal.
| # from opnav team | ||
| def angular_separation(v1, v2): | ||
| dot_prod = np.dot(v1, v2) | ||
| mag1 = np.linalg.norm(v1) | ||
| mag2 = np.linalg.norm(v2) | ||
| return np.arccos(dot_prod / (mag1 * mag2)) |
There was a problem hiding this comment.
I would put this function into its own file and import wherever it's needed - I'm sure that generating measurements for UKF testing is not the only place we would need a function like this
There was a problem hiding this comment.
Ideally, we don't use this function and instead use calculate_cam_measurements in core/opnav.py. Rn there is a fix to make it not a private function, but it is in two other PRs that are still in review. For now, you could replicate the changes made in the other branches or wait until one gets merged so that you can pull from master. I know its not the best, but ¯\_(ツ)_/¯
There was a problem hiding this comment.
Ah. Alternatively: make a tiny PR just for making calculate_cam_measurements in core/opnav.py not private, merge that into main within the next 5 minutes, then merge main into the branches that need it.
There was a problem hiding this comment.
Get rid of this function and use calculate_cam_measurements
| if dt != -1: | ||
| # add length(traj) steps with dt | ||
| num_rows = d_traj.shape[0] | ||
| t_lst = [i*dt for i in range(num_rows)] | ||
| d_traj.insert(0, column='t', value=t_lst) |
There was a problem hiding this comment.
Do we ever calculate the absolute time of the trajectory?
There was a problem hiding this comment.
Is that the epoch? If so, I think its given for some trajectories such as the ones generated with Orekit that Dr. Muhlberger and the undergrad Opnav team is using. Its not needed for the ukf functions so I didn't consider it here
There was a problem hiding this comment.
Hmm ok. The way orbital trajectories make sense for me is that your absolute timing is an essential component of the state of the system because it encodes for the ephemerides of the Sun/Moon. For UKF analysis/development purposes, this is fine, but for flight acceptance testing we will need to consider realistic timings.
| a1 = angular_separation(e_dir, m_dir) | ||
| a2 = angular_separation(e_dir, s_dir) | ||
| a3 = angular_separation(m_dir, s_dir) |
There was a problem hiding this comment.
Can we rename a1 a2 a3 to something more self-documenting
| d_traj.insert(0, column='t', value=t_lst) | ||
|
|
||
| # new "pre_opnav" trajectory should be what opnav-sim expects | ||
| d_traj.to_csv(os.path.join(traj_path, 'pre_opnav.csv'), float_format="%e", index=False) |
There was a problem hiding this comment.
Is there a reason to write this to csv? It might make all our function I/O a bit easier if we passed data frames/jsons from one function to another instead of filenames. I could be very wrong. Is it possible/useful to return the d_traj dataframe instead of writing it to a file?
There was a problem hiding this comment.
This applies not just to this function but to pretty much everything (including the rest of opnav)
There was a problem hiding this comment.
I'm pretty sure this is used to change the formatting of Sean's trajectories so that they are able to be interpreted correctly by the opnav sim. The opnav sim takes in a file, so I think what @mm2774 is doing is fine
There was a problem hiding this comment.
Yep, I was mostly trying to keep opnav-sim untouched. Right now opnav-sim's main loop parses a csv, but we could instead parse data frames in the main loop. And in the arg-parsing function I've added we take a csv, convert it to a dataframe, and call the opnav sim with that dataframe. That way a user can call opnav-sim from the terminal with just the csv file to maintain previous functionality, but inside our code we could pass around dataframes as I/O. It will involve modifying the opnav-sim's logic which could be handled in a future PR
| # main(TEST_C1_DISCRETIZED, 60) | ||
| test_traj_generation() |
There was a problem hiding this comment.
I'm not a fan of commenting out code to change a program's behavior, but for this utility function it's borderline OK
|
|
||
| argparse.add_argument('input', help='name of csv file in directory of the sim, w/o extension') | ||
|
|
||
| argparse.add_argument('-g', action='store_true', help='set -g flag to generate images') |
|
Might also help to add some sort of progress bar to the opnav sim based on the timestamp we're currently evaluating. Not necessary, but just a though on how to provide some feedback for the user |
There was a problem hiding this comment.
👍I did the formatting and had to make some small changes to make the tqdm work. Lmk if anything broke with those changes, but if not then you're good to squash and merge! Great stuff!
Idk if it's possible, but is there a way to change the tqdm progress bar to say iterations instead of it? (I'm assuming that's what it refers to)
Summary
This PR addresses Jira ticket CISLUNAR-307
For a given trajectory, the UKF requires camera measurements including angular size and angular separation (in radians) which can be retrieved from running Dr. Muhlberger's opnav-sim on the same trajectory. The given trajectory with the resulting camera measurements serves as the truth trajectory which can be passed into the UKF for testing purposes.
Previously we've had trajectories such as C1_discretized and 6_Hours used for UKF testing in Spring 2020. The camera measurements for these trajectories were measured in pixels, which since December 2021 have been replaced in favor of radians (though some references to pixels remain). Additionally, the Spring 2020 trajectories' units do not correspond to the opnav-sim's units. Lastly, the trajectory themselves are split into 4 different CSVs.
In this PR I've added a trajectory generation program inside the OpticalNavigation/tests directory. This program takes in a trajectory (such as C1_discretized), pre-processess it for the opnav-sim, runs it through the opnav-sim, processes the output JSON from the sim, and lastly post-processes the trajectory to create a final, UKF ready trajectory. This trajectory is very similar to the original trajectory, except there's an added time column and the newly added camera measurements are in radians.
Running the trajectory through opnav-sim required adding some argument parsing functionality in the sim, which I've also added. This addition preserves the current behavior of opnav-sim as much as possible and it does not rely on the trajectory generation program. For just using the sim, the user can provide a trajectory name along with a "-g" flag to indicate whether or not to generate images. The sim will take in the trajectory and output the results in side the data directory. The -g flag was added for performance considerations when creating trajectories for the UKF.
Testing
I've added a test at the end of the program. This function takes in the c1_discretized trajectory and compares the output of trajectory generation CSV with the original trajectory CSV. Users can also substitute their own trajectory and compare the results as a sanity check.
Testing for camera measurements across pixels and radians is not done here. We assumed that validation of the opnav-sim was sufficient to ensure the camera measurement values are correct.
This program has also been used to recreate the Spring 2020 UKF experiments with success. Experiments for further UKF testing are in progress and may be submitted in a future PR.
Notes
I've moved the angular_separation function from test_ukf_functions.py (created by Emerald, Tanya, Tyler, Rohit) into the trajectory generation file. The idea was that since their file is a unit test and will be using trajectory generation, and the function is used by both files, it should be moved to either a third file or the trajectory generation file.
The arg parser for the opnav-sim might not be necessary at all. Its not needed for trajectory generation and if its not needed for non-UKF related usage I can remove it. But the newly added parameters to the opnav-sim function will be necessary for trajectory generation.
*Formatting was not handled in the commits
Comments