-
Notifications
You must be signed in to change notification settings - Fork 22
I-ALiRT - SWE fixes #2517
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
I-ALiRT - SWE fixes #2517
Conversation
4c0c086 to
5fa99a6
Compare
| grouped_data = find_groups( | ||
| accumulated_data, (0, 59), "swe_seq", "met", "swe_nom_flag" | ||
| ) |
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.
Why are we doing this in two locations? We have a find that takes in the flag and the filter_valid_groups which takes in the flag. Since this is only swe-specific at this point what if you put it right after this instead?
# Drop any off-nominal SWE groups
grouped_data = grouped_data.where(
filtered_data["swe_nom_flag"] != 0,
drop=True,
)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.
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.
For the data grouping I use the swe_seq=0 to determine where the group begins. I also use the src_seq_ctr to ensure that the group is complete. So if I remove the line shown above initially then I end up throwing out the entire group.
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.
Interesting, this brings up a question about whether you want to be using src_seq_ctr at all in this case then? Wouldn't you want to only group by the swe_seq instead? Otherwise I'd think you might get 0-59 src_seq_ctr groupings, but that would only have 59 values in it and the 60th value is in the next group potentially.
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.
For the src_seq_ctr I only filter out groups with non-sequential src_seq_ctr values. It is a final check to make certain that we have the correct group. Since I am using a common tool for all instruments to check the group this was particularly important for the MAG instrument based on their packet definition. But it also doesn't hurt to check the other instruments. In this case I do think that we should keep it.
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 still not completely following. Could you add a test for this without all 1s?
I think that if you get events and it goes [0, 1, 2, 3, 0, 4, 5, 6], there would be some confusion about where the groups are starting with the 0s in it. The code currently looks like you're filtering that middle 0 out at the end, but how do you know you have the right number of events in the first place versus seeing "0, 1, 2, 3" and saying that isn't enough for a group, then seeing "0, 4, 5, 6" and saying that isn't a complete group either so tossing the entire sequence out.
Overall, I'd just suggest that this might not be one where we want to re-use so much code and pass-through a lot of keywords that are only used deep down in a routine for one specific instrument.
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.
This part should have taken care of that concern:
start_times = sorted_data[time_name][
(sorted_data[sequence_name] == sequence_range[0]) & (sorted_data[flag] != 0)
]
But I have now changed the code so that the flagged events are now filtered prior to entering the function and the sequential sequence counter is not being used for SWE. I think it would have been fine either way, but it is good to keep things simple. I have also added a test.
|
@greglucas I'm back to this PR. Apologies I didn't look at it after AGU. Hopefully it's not too difficult looking at it again after so long. |
greglucas
left a comment
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.
Thank you! That is easier for me to follow. I'm still not sure I'm 100% following the flow-through since it has been a while, but this reads better to me.
93fb8c9
into
IMAP-Science-Operations-Center:dev

Change Summary
Overview
The SWE I-ALiRT data is based on a spin period that is exact. During commissioning the spin has been varying. The resolution to this was to create a packet with zero data for all SWE variables, including the variable "swapi_seq_number". The sequence numbers are used to group the data so having duplicate values of "0" was causing the data to be thrown out. This PR addresses that issue.
Updated Files
Testing