-
Notifications
You must be signed in to change notification settings - Fork 19
feat: add more flexibility when inserting default iir params #1221
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with these changes although I would prefer that in addition to adding these optional arguments you add the known time range for the lat to the time_range dictionary. I also think a simple extension of this is to allow the time_range dictionary to contain a list of tuples of start/end times and loop over that list to allow for multiple epochs. The latter you don't need to implement but I would like you to implement the former.
Yeah having some known time ranges would be good. I don't currently have them recorded in a convenient way. @mmccrackan you have a list of bad obs right? Do you have ranges you recommend? |
I only know it started after 04/25 and no Mars observations were missing them before that. It is not only Mars observations, but I also saw some rcw38 that are missing them as well. I will run a script to get a complete list of the ones with missing values. |
Here's the full list of
|
Confirm that the end date is around when I slacked @tpsatt about hard hammering to reset these. |
Okay @msilvafe I think I got those changes in. Let me know if it looks okay to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lat time_ranges has typo. Other points look good to me.
Good catch! Fixed |
No description provided.