PriorityScheduler: Update _get_filled_indices() to avoid 'block already occupied' ValueError #550
Conversation
| filled = np.where((start_end[0]-0.5*u.second < times) & (times < start_end[1])) | ||
| is_open_time[filled[0]] = False |
There was a problem hiding this comment.
I'd bet that fixing 0.5 s is fine in most cases, but maybe not all cases. I think it would be more correct and safer to raise a warning when np.diff(start_end) <= np.min(np.diff(times)). What do you think?
There was a problem hiding this comment.
Including such a warning is probably good. Currently, I can't imagine a scenario where both time_resolution and duration are less than 1 second, which is why I opted to simply halve it. Consider revising to something like:
filled = np.where((start_end[0]-0.05*u.second <= times) & (times < start_end[1]))
Because in general I think we should mark "as filled" on one side when equal. Given that this is likely akin to floating point comparisons, subtracting a tolerance here seems logical. The addition of "<=" may provide further clarity to our intentions.
| if len(filled[0]) > 0: | ||
| is_open_time[filled[0]] = False | ||
| is_open_time[min(filled[0]) - 1] = False | ||
| filled = np.where((start_end[0]-0.5*u.second < times) & (times < start_end[1])) |
There was a problem hiding this comment.
| filled = np.where((start_end[0]-0.5*u.second < times) & (times < start_end[1])) | |
| if np.diff(start_end) <= np.min(np.diff(times)): | |
| warnings.warn( | |
| "Unexpected behavior may occur when the time " | |
| f"resolution ({np.min(np.diff(times))}) " | |
| "is not smaller than the block duration " | |
| f"({np.diff(start_end)}).", AstroplanWarning | |
| ) | |
| filled = np.where((start_end[0]-0.5*u.second < times) & (times < start_end[1])) |
If you accept this change, you'll also need to add the following import to this file:
from .exceptions import AstroplanWarning
…dy occupied' ValueError Previously, for blocks with a duration equal to the time_resolution, _get_filled_indices() was unable to identify overlapping slots. This was due to the comparison of start times not accounting for these specific cases. This has been addressed by adjusting the start_time comparison to be slightly earlier (0.5 seconds). This change enables the correct identification of filled time slots for blocks with minimal duration, thus preventing the 'block already occupied' error.
michaelbaisch
force-pushed
the
fix_block_already_occupied
branch
from
3e874d2
to
ecca3b5
Compare
…on instead of 'np.diff'
Previously, for blocks with a duration equal to the time_resolution, _get_filled_indices() was unable to identify overlapping slots. This was due to the comparison of start times not accounting for these specific cases.
This has been addressed by adjusting the start_time comparison to be slightly earlier (0.5 seconds). This change enables the correct identification of filled time slots for blocks with minimal duration, thus preventing the 'block already occupied' error.
Fixes #368