Appointment Schedule

Question

In the below code there are two data frames given, free_schedule and appointments which represent a pre-defined available timetable for a service, and appointed times for a day in that schedule in that order.

In free_schedule, time column represents the time of an available schedule and quota column represents the number of available appointments for a given time.

In appointments, time column represents the time of the appointment and, quota represents the number of people that have appointment starting from the given time (there's a catch here).

I am given a task to prepare a function, avail_times_for_a_grp_siz that returns a list of times available for a given free-schedule, already appointed appointments and a given size of people.

There's only one catch: In a group of people making an appointment their appointment times have to be one after another, as in if there are a group of 3 people trying to get appointment for 10:00 o'clock, then all three of 10:00, 10:20, 10:40 needs to have quota greater than or equal to 1 on the schedule.

Code:

import pandas as pd


def book_appointments(schedule, appointments):
    """ Books the appointments df in schedule df, and returns new available times in
    the same format as schedule.
    """

    s_df = pd.DataFrame(schedule)
    a_df = pd.DataFrame(appointments)

    pd.options.mode.chained_assignment = None       # to ignore overwriting warning
    for a_index, grp_siz, a_time in a_df.itertuples():
        s_idx_of_time_match = s_df.index[s_df['time'] == a_time].tolist()[0]
        s_df_grp_siz_bfr = s_df[s_idx_of_time_match:s_idx_of_time_match + grp_siz]

        if (s_df_grp_siz_bfr['quota'] > 0).all():
            s_df_grp_siz_bfr['quota'] = s_df_grp_siz_bfr['quota'].apply(lambda a: a-1)
    pd.options.mode.chained_assignment = 'warn'     # put it back to default

    new_schedule = list(s_df.to_dict('records'))    # an updated list of available
    return new_schedule                             # times in the same format.

def bookable_times(schedule, grp_siz):
    """ Returns a list of available times for a group size of grp_siz in schedule.
    """

    s_df = pd.DataFrame(schedule)
    bookable_hours = list()

    for s_index, quota, s_time in s_df.loc[s_df['quota'] > 0].itertuples():
        if (s_df.iloc[s_index:s_index + grp_siz]['quota'] > 0).all():
            bookable_hours.append(s_time)

    return bookable_hours

def avail_times_for_a_grp_siz(schedule, appointments=None, grp_siz=1):
    """ Returns available times for a group size of grp_siz in schedule given that
    appointments is booked in schedule.
    """

    return bookable_times(book_appointments(schedule, appointments), grp_siz)

if __name__ == "__main__":
    try:
        from data2 import free_schedule, appointments
    except:
        print("Warning: data2.py wasn't found!\n")
        free_schedule = [
            {'quota': 3, 'time': '09:00'},
            {'quota': 3, 'time': '09:20'},
            {'quota': 3, 'time': '09:40'},
            {'quota': 3, 'time': '10:00'},
            {'quota': 3, 'time': '10:20'},
            {'quota': 3, 'time': '10:40'},
            {'quota': 2, 'time': '11:00'},
            {'quota': 2, 'time': '11:20'},
            {'quota': 2, 'time': '11:40'}
        ]
        appointments = [
            {'number_of_people': 2, 'time': '10:40'},
            {'number_of_people': 3, 'time': '09:00'},
            {'number_of_people': 1, 'time': '11:40'},
            {'number_of_people': 2, 'time': '11:20'},
            {'number_of_people': 1, 'time': '09:40'},
            {'number_of_people': 3, 'time': '09:00'},
            {'number_of_people': 4, 'time': '10:00'}
        ]
    input(avail_times_for_a_grp_siz(free_schedule, appointments, 2))
    input(avail_times_for_a_grp_siz(free_schedule))

Review Concerns

It satisfies the test condition I am given, in the main section of the code above. However, I am concerned that:

• It can be written in a more understandable and readable manner.
• Perhaps it can be written to satisfy the condition with better performance.
• Perhaps there is a tiny issue that one finds inappropriate or such.

I will submit the above code to be reviewed by my possible future supervising developer, so I need it to be as good as possible and refine it as much as possible before submitting.

Answer 1

Going line by line:

def book_appointments(schedule, appointments):

I always use type hints. With it we can be explicit about what type of data we pass to a function and what we expect to get from it. This is how your signature would look like:

from typing import Union, Dict, List


def book_appointments(schedule: List[Dict[str, Union[int, str]]],
                      appointments: List[Dict[str, Union[int, str]]]
                      ) -> List[Dict[str, Union[int, str]]]:

Looks terrible. Data is structured in a quite complicated way. We will see what we can do about it.

---

""" Books the appointments df in schedule df, and returns new available times in
the same format as schedule.
"""

Few problems here. You exceeded maximum line length which is 72 for docstrings. And appointments and schedule are not dataframes yet. In fact, if you use type hints, then you don't need to specify types of data here. Also you could follow one of the standard docstring formats, for example reST:

"""
Makes booking of appointments in the schedule
:param schedule: some description of schedule
:param appointments: some description of appointments
:return: new available times in the same format as schedule
"""

---

s_df = pd.DataFrame(schedule)
a_df = pd.DataFrame(appointments)

Why do we pack it here? If we will pass these dataframes to book_appointments instead of packing them here, this function will be much cleaner. We will call it like:

book_appointments(pd.DataFrame(schedule),
                  pd.DataFrame(appointments))

And the signature will look like this:

def book_appointments(schedule: pd.DataFrame,
                      appointments: pd.DataFrame
                      ) -> List[Dict[str, Union[int, str]]]:

This is better than before.

---

pd.options.mode.chained_assignment = None       # to ignore overwriting warning

I really don't like the idea of changing pandas settings here. And why not deal with the problem instead of suppressing the warning?

---

for a_index, grp_siz, a_time in a_df.itertuples():
    s_idx_of_time_match = s_df.index[s_df['time'] == a_time].tolist()[0]
    s_df_grp_siz_bfr = s_df[s_idx_of_time_match:s_idx_of_time_match + grp_siz]

    if (s_df_grp_siz_bfr['quota'] > 0).all():
        s_df_grp_siz_bfr['quota'] = s_df_grp_siz_bfr['quota'].apply(lambda a: a-1)

Iterating over rows of pandas dataframe is a sign that probably you are doing something wrong. The key to efficiency with pandas is performing operations for a whole dataframe/series, not go row by row. Exceeding 79 characters line length here as well.
Now, this

s_idx_of_time_match = s_df.index[s_df['time'] == a_time].tolist()[0]

can be rewritten in a vectorized way:

time_match_indices = schedule['time'].searchsorted(appointments['time'])

Be careful with the following line:

s_df_grp_siz_bfr = s_df[s_idx_of_time_match:s_idx_of_time_match + grp_siz]

If s_idx_of_time_match + grp_siz will exceed the last index, you won't get any warning. I don't know if you wanted that or not.
Also, here is the root of your problem with the SettingwithCopyWarning. You could use .loc or .iloc.
Furthermore, I think you would want to operate on a copy of a schedule, not the original dataframe. If so, do this:

new_schedule = schedule.copy()

And this:

s_df_grp_siz_bfr['quota'] = s_df_grp_siz_bfr['quota'].apply(lambda a: a-1)  

is the same as:

s_df_grp_siz_bfr['quota'] -= 1

Finally, apart from that vectorized code from above, the rest is also vectorizable. But I believe the resulting code will be quite sophisticated and not really readable. This brings me to the point that probably you shouldn't use pandas at all. But see for yourself. How big is your data? What advantages of pandas you are going to use that simple lists or dictionaries don't have.

---

new_schedule = list(s_df.to_dict('records'))    # an updated list of available
return new_schedule                             # times in the same format.

First of all, in this case it is enough to write just

return list(s_df.to_dict('records')) 

Don't separate inline comments like this. If you really want to write them, do it like this:

# Updated list of available times in the same format
return list(s_df.to_dict('records')) 

But in fact you don't need it at all because you already mentioned returned data in your docstring.
And again, we could just return dataframe and convert it to a dictionary outside of this function (if you really need it).

---

def bookable_times(schedule, grp_siz):  

Missing 2 blank lines before definition according to PEP 8.
grp_siz could be named as group_size. It is not old Fortran to care about lengths of variables names.

---

s_df = pd.DataFrame(schedule)

Again, pass this schedule as dataframe to this function, don't pack it here.

---

bookable_hours = list()

for s_index, quota, s_time in s_df.loc[s_df['quota'] > 0].itertuples():
    if (s_df.iloc[s_index:s_index + grp_siz]['quota'] > 0).all():
        bookable_hours.append(s_time)

return bookable_hours

Not the best usage of pandas here as well. Vectorizing it can be quite difficult. It's also possible to use list comprehension here:

positive_quotas_schedule = schedule.loc[schedule['quota'] > 0].itertuples()
return [time
        for index, quota, time in positive_quotas_schedule
        if (schedule.iloc[index:index + group_size]['quota'] > 0).all()]

It will be even better if you make a generator out of this function.

---

I think you will figure out the rest. If you have any questions, please ask. And good luck with submitting your code.

Answer 2

As is, it allows unlimited group size to apply when there are no appointments, to prevent that and also not to allow 11:40 to 9:00 2 people appointments and for more stability I've made the following additions in bookable_times:

last_idx_with_avail_quota = (s_index + grp_siz)
if last_idx_with_avail_quota <= s_df.shape[0]:  # don't allow circularity

resulting in:

import pandas as pd


def book_appointments(schedule, appointments):
    """ Books valid appointments in schedule, and returns new availabletimes in the
    same format as schedule.
    """

    s_df = pd.DataFrame(schedule)
    a_df = pd.DataFrame(appointments)

    pd.options.mode.chained_assignment = None       # to ignore overwriting warning
    for a_index, grp_siz, a_time in a_df.itertuples():
        s_idx_of_time_match = s_df.index[s_df['time'] == a_time].tolist()[0]
        s_df_grp_siz_bfr = s_df[s_idx_of_time_match:s_idx_of_time_match + grp_siz]

        if (s_df_grp_siz_bfr['quota'] > 0).all():   # -1 quotas if rq. times are avail.
            s_df_grp_siz_bfr['quota'] = s_df_grp_siz_bfr['quota'].apply(lambda a: a-1)
    pd.options.mode.chained_assignment = 'warn'     # put it back to default

    new_schedule = list(s_df.to_dict('records'))    # an updated list of available
    return new_schedule                             # times in the same format.

def bookable_times(schedule, grp_siz):
    """ Returns a list of available times for a group size of grp_siz in schedule.
    """

    s_df = pd.DataFrame(schedule)
    bookable_hours = list()

    for s_index, quota, s_time in s_df.loc[s_df['quota'] > 0].itertuples():
        last_idx_with_avail_quota = (s_index + grp_siz)
        if last_idx_with_avail_quota <= s_df.shape[0]:  # don't allow circularity
            if (s_df.iloc[s_index:last_idx_with_avail_quota]['quota'] > 0).all():
                bookable_hours.append(s_time)

    return bookable_hours

def avail_times_for_a_grp_siz(schedule, appointments=None, grp_siz=1):
    """ Returns available times for a group size of grp_siz in schedule given that
    appointments is booked in schedule.
    """

    return bookable_times(book_appointments(schedule, appointments), grp_siz)

if __name__ == "__main__":
    try:
        from data2 import free_schedule, appointments
    except:
        print("Warning: data2.py wasn't found!\n")
        free_schedule = [
            {'quota': 3, 'time': '09:00'},
            {'quota': 3, 'time': '09:20'},
            {'quota': 3, 'time': '09:40'},
            {'quota': 3, 'time': '10:00'},
            {'quota': 3, 'time': '10:20'},
            {'quota': 3, 'time': '10:40'},
            {'quota': 2, 'time': '11:00'},
            {'quota': 2, 'time': '11:20'},
            {'quota': 2, 'time': '11:40'}
        ]
        appointments = [
            {'number_of_people': 2, 'time': '10:40'},
            {'number_of_people': 3, 'time': '09:00'},
            {'number_of_people': 1, 'time': '11:40'},
            {'number_of_people': 2, 'time': '11:20'},
            {'number_of_people': 1, 'time': '09:40'},
            {'number_of_people': 3, 'time': '09:00'},
            {'number_of_people': 4, 'time': '10:00'}
        ]
    input(avail_times_for_a_grp_siz(free_schedule, grp_siz=9))
    df = pd.DataFrame(book_appointments(free_schedule, appointments))
    input(df)
    input(avail_times_for_a_grp_siz(free_schedule))