Fixing issue #8770: Improved frequency parameter logic to set it to 'D' only if periods, start, or end are None.

[rjavierch]

This Pull Request addresses the issue reported regarding the default setting of the frequency (freq) parameter in Xarray. Currently, the frequency is explicitly set to "D" by default, which may not always be appropriate behavior. The proposed improvement adjusts the logic to set the frequency to "D" only if any of the parameters (periods, start, or end) are None, providing more flexibility and aligning with expected behavior.

Changes:

Updated logic for setting the frequency parameter to consider periods, start, or end values.
Added checks to ensure that the frequency is set to "D" only when necessary.

Please review and provide feedback on the proposed changes. Any suggestions for further improvements are welcome.

• Closes Error in xr.date_range with exactly three parameters provided. #8770

[welcome]

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

[mathause]

Looks good. Can you add a small test (let us know if you need support for that) and maybe an entry to the whats-new doc.

[rjavierch]

Looks good. Can you add a small test (let us know if you need support for that) and maybe an entry to the whats-new doc.

Oh, yes, I would like to receive the support for that. I would love to add a small test and an entry to the whats-new doc.

[spencerkclark]

Indeed thanks @rjavierch—just one minor rethink on my part. In terms of testing, you could add a test similar to:

xarray/xarray/tests/test_cftime_offsets.py

Lines 1730 to 1735 in 8e1dfcf

	def test_cftime_range_same_as_pandas(start, end, freq):
	result = date_range(start, end, freq=freq, calendar="standard", use_cftime=True)
	result = result.to_datetimeindex()
	expected = date_range(start, end, freq=freq, use_cftime=False)
	np.testing.assert_array_equal(result, expected)

Only in this case you would test the situation where freq is not provided as an argument to date_range, but start, end, and periods are.

[spencerkclark]

Sorry on second thought could you move this logic into cftime_range (defined a little farther up in this file)? That way we can harmonize the default arguments across pandas.date_range, xarray.cftime_range, and xarray.date_range.

[rjavierch]

Thanks for the feedback! I moved the logic from date_range to cftime_range since indeed, it was redundant in date_range as pandas should handle it. Now the lines are in cftime_range as it lacks the logic.
Also, the tests were also included.

However, I wondered whether to change the docstrings defaults from "D" to None. Pandas date_range function has default input parameter None but the docstring says the default is "D" pandas.date_range.

[mathause]

However, I wondered whether to change the docstrings defaults from "D" to None. Pandas date_range function has default input parameter None but the docstring says the default is "D" pandas.date_range.

No strong opinion. I would probably have kept it as is (it's good enough for pandas...)

---

There is a failing test, because only passing start and end works now. Can you update the test_invalid_cftime_range_inputs by removing the line:

-("2000", "2001", None, None, None),

Your new tests are good, but they don't check the issue you reported in #8770, yet. Can you add a test that ensures the following is valid:

xr.cfdate_range(start="2001-01-01", end="2002-01-01", periods=2)

(and also for date_range)

[rjavierch]

Thanks for the feedback! I have updated the test_invalid_cftime_range_inputs inputs and added the periods argument as input in tests test_cftime_range_no_freq and test_date_range_no_freq.

[mathause]

LGTM

[rjavierch]

Perfect! I close the issue with your approval.

[mathause]

I reopen the PR - we still have to merge it 🙂 we can do that soon, but I want to give others the chance to have a look if they want

[mathause]

Failure is unrelated.

[spencerkclark]

Looks good to me too—thanks @rjavierch!

[rjavierch]

I reopen the PR - we still have to merge it 🙂 we can do that soon, but I want to give others the chance to have a look if they want

Oh true, my bad! if there is something extra I need to do, please do not hesitate to let me know.

[spencerkclark]

All good @rjavierch—merging now.

[welcome]

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again!