Skip to content

PriorityScheduler: Update _get_filled_indices() to avoid 'block already occupied' ValueError #550

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

PriorityScheduler: Update _get_filled_indices() to avoid 'block already occupied' ValueError #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0723f3e
PriorityScheduler: Update _get_filled_indices() to avoid 'block alrea…
michaelbaisch May 29, 2023
ff6bcd5
In '_get_filled_indices()' add warning if block's duration <= resolut…
michaelbaisch Sep 21, 2023
ecca3b5
Add changelog entry for #550
michaelbaisch Sep 21, 2023
c28a852
Improve the performance of '_get_filled_indices()' by using subtracti…
michaelbaisch Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
0.10 (unreleased)
-----------------

- Fix ``block already occupied`` error in ``PriorityScheduler`` by adjusting
the condition for filled slots in ``_get_filled_indices()``. This was primarily
an issue for blocks with ``duration`` <= ``time_resolution``. [#550]

0.9.1 (2023-09-20)
------------------
Expand Down
16 changes: 12 additions & 4 deletions astroplan/scheduling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import copy
from abc import ABCMeta, abstractmethod
import warnings

import numpy as np

Expand All @@ -18,6 +19,7 @@
from .utils import time_grid_from_range, stride_array
from .constraints import AltitudeConstraint
from .target import get_skycoord
from .exceptions import AstroplanWarning

__all__ = ['ObservingBlock', 'TransitionBlock', 'Schedule', 'Slot',
'Scheduler', 'SequentialScheduler', 'PriorityScheduler',
Expand Down Expand Up @@ -705,15 +707,21 @@ def __init__(self, *args, **kwargs):

def _get_filled_indices(self, times):
is_open_time = np.ones(len(times), bool)
times_resolution = np.min(times[1:] - times[:-1])
# close times that are already filled
pre_filled = np.array([[block.start_time, block.end_time] for
block in self.schedule.scheduled_blocks if
isinstance(block, ObservingBlock)])
for start_end in pre_filled:
filled = np.where((start_end[0] < times) & (times < start_end[1]))
if len(filled[0]) > 0:
is_open_time[filled[0]] = False
is_open_time[min(filled[0]) - 1] = False
if start_end[1] - start_end[0] <= times_resolution:
warnings.warn(
"Unexpected behavior may occur when the time "
f"resolution ({times_resolution.to(u.second)}) "
"is not smaller than the block duration "
f"({np.diff(start_end)[0].to(u.second)}).", AstroplanWarning
)
filled = np.where((start_end[0]-0.5*u.second < times) & (times < start_end[1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

is_open_time[filled[0]] = False
Comment on lines +723 to +724
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return is_open_time

def _make_schedule(self, blocks):
Expand Down