Skip to content
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.

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

API: Rm default value from time_delta for cd_index. #6953

Merged
merged 6 commits into from Oct 5, 2023

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Sep 26, 2023

I'd like to propose a change to the cd_index API: removing the default value for time_delta. The current default of 5 years as a timedelta object feels arbitrary, and I can't think of a strong reason why a default value makes sense here. Furthermore, providing a datetime.timedelta object suggests that the time node attribute must be datetime.datetime objects by default, which feels like too strong a recommendation IMO.

@MridulS
Copy link
Member

MridulS commented Sep 26, 2023

I agree we should let users decide this bit. We should also update the error raised in line 106-108, there is no default datetime type anymore.

@rossbar
Copy link
Contributor Author

rossbar commented Oct 3, 2023

We should also update the error raised in line 106-108, there is no default datetime type anymore.

Great catch! Should be fixed in 4048403.

@dschult
Copy link
Member

dschult commented Oct 4, 2023

The failures here are not related... And geospatial example somehow doubled the size of its array.

And because I tried to use a github comment "suggestion" instead of just pushing to this PR, ended up with two commits instead of one and my test was hidden (unless you click "show resolved"). So here's that text:

I'm wondering if we could add an example that doesn't use datetime. I tried quickly to take this example and make the dates just a year as an int e.g. '2012'. To get it to work, I had to make time_delta=4 instead of time_delta=5. Did I miss something? Or maybe there is an "off-by-one" error in the 5*365 value (or in how time_delta is used).

@dschult dschult force-pushed the propose-cd-index-delta-varpos branch from e8e77e7 to 4a9bd8b Compare October 4, 2023 23:58
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

I think this is ready for merging (or at least review). It improves the current interface and improves the docs.

@jarrodmillman jarrodmillman added this to the 3.2 milestone Oct 5, 2023
@MridulS MridulS merged commit ba11717 into networkx:main Oct 5, 2023
38 checks passed
Alex-Markham pushed a commit to Alex-Markham/networkx that referenced this pull request Oct 13, 2023
* API: Rm default value from time_delta for cd_index.

* Update exception message.

Co-authored-by: Mridul Seth <git@mriduls.com>

* TST: modify pytest.raises regex.

* Second cd_index example with integer times

* typo

* warn about leap years

---------

Co-authored-by: Mridul Seth <git@mriduls.com>
Co-authored-by: Dan Schult <dschult@colgate.edu>
dschult added a commit to BrunoBaldissera/networkx that referenced this pull request Oct 23, 2023
* API: Rm default value from time_delta for cd_index.

* Update exception message.

Co-authored-by: Mridul Seth <git@mriduls.com>

* TST: modify pytest.raises regex.

* Second cd_index example with integer times

* typo

* warn about leap years

---------

Co-authored-by: Mridul Seth <git@mriduls.com>
Co-authored-by: Dan Schult <dschult@colgate.edu>
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* API: Rm default value from time_delta for cd_index.

* Update exception message.

Co-authored-by: Mridul Seth <git@mriduls.com>

* TST: modify pytest.raises regex.

* Second cd_index example with integer times

* typo

* warn about leap years

---------

Co-authored-by: Mridul Seth <git@mriduls.com>
Co-authored-by: Dan Schult <dschult@colgate.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants