frequencies: Number of pivot points differs for same date range when both start/end date are inclusive

[victorlin]

Current Behavior

Setup:

from augur.frequency_estimators import get_pivots
from augur.dates import numeric_date

Getting monthly pivots between 2022-01-02 and 2022-03-02 gives 2 pivots:

start_date = numeric_date('2022-01-02')
end_date = numeric_date('2022-03-02')
get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=1, pivot_interval_units="months")
# out: array([2022.0833, 2022.1667])
# == [2022-02-01, 2022-03-01]

However, shifting back one day to 2022-01-01 and 2022-03-01 gives 3 pivots:

start_date = numeric_date('2022-01-01')
end_date = numeric_date('2022-03-01')
get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=1, pivot_interval_units="months")
# out: array([2022.    , 2022.0833, 2022.1667])
# == [2022-01-01, 2022-02-01, 2022-03-01]

Expected behavior

The same date range should produce the same number of pivots (i.e. 2022-01-01 and 2022-03-01 should still give 2 pivots).

Explanation

This is due to 2 behaviors in current usage of pd.date_range():

1. The pivot points are fixed at the start of every month:
   

   augur/augur/frequency_estimators.py

   Lines 56 to 57 in f68c448

   	if pivot_interval_units == "months":
   	offset = "%sMS" % pivot_interval

2. The default parameter of closed=None to pd.date_range() means the interval is inclusive of both start_date and end_date.

It seems like this behavior is intentional, given the test here:

augur/tests/test_frequencies.py

Lines 85 to 93 in f68c448

	def test_get_pivots_by_months():
	"""Get pivots where intervals are defined by months.
	"""
	pivots = get_pivots(observations=[], pivot_interval=1, start_date=2015.0, end_date=2016.0, pivot_interval_units="months")
	# Pivots should include all 12 months of the year plus the month represented
	# by the end date, since the pandas month interval uses "month starts". See
	# pandas date offsets documentation for more details:
	# https://pandas.pydata.org/pandas-docs/stable/user_guide/timeseries.html#offset-aliases
	assert len(pivots) == 13

Possible solution

In the call to pd.date_range()

augur/augur/frequency_estimators.py

Lines 63 to 66 in f68c448

	datetime_pivots = pd.date_range(
	float_to_datestring(pivot_start),
	float_to_datestring(pivot_end),
	freq = offset

specify either closed='left' or closed='right' so the range is inclusive on only one side. However, I'm not sure which is more meaningful for the application here. It also breaks a few existing tests, see failing tests on victorlin@1487790.

@huddlej what do you think of this approach?

Your environment: if running Nextstrain locally

• Version (e.g. auspice 2.7.0): Augur 15.0.2

[victorlin]

@huddlej a similar problem is causing CI to fail today, this time being the last day of the month. A small snippet to repro:

from augur.frequency_estimators import get_pivots
from augur.dates import numeric_date
from treetime.utils import datestring_from_numeric

start_date = numeric_date('2022-01-01')
end_date = numeric_date('2022-03-01')
pivots = get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=1, pivot_interval_units="months")
[datestring_from_numeric(date) for date in pivots]
# ['2022-01-01', '2022-02-01', '2022-03-01']

start_date = numeric_date('2022-01-31')
end_date = numeric_date('2022-03-31')
pivots = get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=1, pivot_interval_units="months")
[datestring_from_numeric(date) for date in pivots]
# ['2022-02-28', '2022-03-31']

[victorlin]

Update: we discussed this on Slack and it turns out this is a side effect of the way the duration is applied on incrementally updated pivot points.

augur/augur/frequency_estimators.py

Lines 79 to 82 in e12dbe1

	pivot = end
	while pivot >= start:
	pivots.appendleft(pivot)
	pivot = pivot - delta

The proper solution is to determine pivots based on the original end instead of intermediate pivot points. This should be easy with the multiplier operator on isodate.Duration.

[victorlin]

Well something related cropped up again on today, March 31st. Converted the failing test into an example here:

from augur.frequency_estimators import get_pivots
from augur.dates import numeric_date
from treetime.utils import datestring_from_numeric

start_date = numeric_date('2022-03-30')
end_date = numeric_date('2022-09-30')
pivots = get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=3, pivot_interval_units="months")
[datestring_from_numeric(date) for date in pivots]
# ['2022-03-30', '2022-06-30', '2022-09-30']

start_date = numeric_date('2022-03-31')
end_date = numeric_date('2022-09-30')
pivots = get_pivots(observations=[], start_date=start_date, end_date=end_date, pivot_interval=3, pivot_interval_units="months")
[datestring_from_numeric(date) for date in pivots]
# ['2022-06-30', '2022-09-30']

The reason is that subtracting monthly units from 2022-09-30 won't produce any dates with the 31 as the day part.

---

I think this is reasonable behavior. Given the purpose of the failing test:

this test checks the logical consistency of the requested relative dates and pivot interval

and that we can't expect logical consistency, I'd propose to remove this test since relative dates and pivot intervals are already covered by pytests (1, 2).