Skip to content

Conversation

@subagonsouth
Copy link
Contributor

Change Summary

Overview

Adds the following goodtimes check functions and test coverage:

  • drop_incomplete_spin_sets
  • drop_drf_times

Updated Files

  • imap_processing/hi/hi_goodtimes.py
    • create_goodtimes_dataset function updated to include all unique MET times. Times with missing packets are removed in the new drop_incomplete_spin_sets function.
    • Add drop_incomplete_spin_sets function.
    • Add `drop_drf_times function
  • imap_processing/tests/hi/test_hi_goodtimes.py
    • Add test coverage for new functions

Part 1 of #2540

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements goodtimes filtering algorithms from the IMAP-Hi project by adding two new culling functions (drop_incomplete_spin_sets and drop_drf_times) and updating the goodtimes dataset creation logic to include all unique MET timestamps instead of filtering at creation time.

Key Changes:

  • Modified create_goodtimes_dataset to include all unique MET values, deferring filtering to culling functions
  • Added drop_incomplete_spin_sets to filter incomplete 8-spin histogram periods
  • Added drop_drf_times to remove data during spacecraft drift restabilization periods

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
imap_processing/hi/hi_goodtimes.py Updated create_goodtimes_dataset to keep all unique METs; added drop_incomplete_spin_sets and drop_drf_times culling functions; enhanced remove_times to handle edge cases with single MET values
imap_processing/tests/hi/test_hi_goodtimes.py Updated existing tests to reflect new behavior (12 METs instead of 10); added comprehensive test suites for both new functions covering complete/incomplete spin sets, DRF transitions, edge cases, and custom cull codes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

subagonsouth and others added 2 commits January 6, 2026 10:53
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

The code looks good to me. One thing I was slightly confused by is the naming though. Generally "drop" means remove it from the dataset to me, so I was a bit confused how things were being updated and what was being returned vs in-place. I think what is happening though is you are identifying/labeling bad times, not actually removing them from the dataset. So now those are "marked" with the cull_flags. In the tests you're comparing that the cull_flags were set, not that those items aren't present anymore.

What about mark_incomplete_spins() or something like that for naming rather than drop/filter?

@subagonsouth
Copy link
Contributor Author

@greglucas

The code looks good to me. One thing I was slightly confused by is the naming though. Generally "drop" means remove it from the dataset to me, so I was a bit confused how things were being updated and what was being returned vs in-place. I think what is happening though is you are identifying/labeling bad times, not actually removing them from the dataset. So now those are "marked" with the cull_flags. In the tests you're comparing that the cull_flags were set, not that those items aren't present anymore.

What about mark_incomplete_spins() or something like that for naming rather than drop/filter?

Yes, I see the confusion there. The goodtimes object is really just marking times that are not good, as you pointed out. Then when the text file gets written, only the remaining unmarked goodtimes are written out. I will update as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants