Update auto follow-up request priority #252
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does:
Also, a minor change to try to get around the current issues with missing data: When building the "all_prv_candidates"/alert_history list used for ML feature generation, also append the full history of alerts, not just prv_candidates. This should help BTSbot not have high scores on old variables.
A few questions for the reviewers:
@mcoughlin is it correct that updating a request's priority (so effectively, the payload) tells the instrument of the allocation to update it? Just making sure it doesn't just give us the illusion to do so. If that's the case, we might want to delete and recreate instead, which comes with it's own challenges.
-> UPDATE: I checked and it should! At least this is implemented for SEDM's API class.
@nabeelre, given the additional cut on group_id when looking for existing requests, this means that if SEDM has pending or completed requests with either another allocation, or the same allocation but not shared with the auto-triggering group where you have your filter (which should be the case for existing requests), or has a different payload, ... and hasn't been classified and didn't get a spectra, we might still trigger SEDM. Just wanted to make sure that this is the expected behavior. I think it is, given the data access constraints we talked about earlier.