1030 option for auto typecasting datediff #1162

aliceoleary0 · 2023-03-31T11:09:10Z

Adding date-casting so date comparisons can accept date inputs as strings

…tion Linker method for Spark

…to tweak settings initialization Linker method for Spark

github-actions · 2023-03-31T11:10:27Z

Test: test_2_rounds_1k_duckdb

Percentage change: -31.4%

	date	time	stats_mean	stats_min	commit_info_branch	commit_info_id	machine_info_cpu_brand_raw	machine_info_cpu_hz_actual_friendly	commit_hash
849	2022-07-12	18:40:05	1.89098	1.87463	splink3	`c334bb9`	Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz	2.7934 GHz	`c334bb9`
1587	2023-04-24	15:21:04	1.31726	1.28662	(detached head)	`12f22ce`	Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz	2.5939 GHz	`12f22ce`

Test: test_2_rounds_1k_sqlite

Percentage change: -24.5%

	date	time	stats_mean	stats_min	commit_info_branch	commit_info_id	machine_info_cpu_brand_raw	machine_info_cpu_hz_actual_friendly	commit_hash
851	2022-07-12	18:40:05	4.32179	4.25898	splink3	`c334bb9`	Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz	2.7934 GHz	`c334bb9`
1589	2023-04-24	15:21:04	3.21643	3.21365	(detached head)	`12f22ce`	Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz	2.5939 GHz	`12f22ce`

Click here for vega lite time series charts

RossKen · 2023-03-31T15:32:19Z

.vscode/launch.json

Remove this file

RossKen · 2023-03-31T15:32:28Z

.DS_Store

Remove this file

docs/topic_guides/customising_comparisons.ipynb

RossKen

Really good work! 🎉 Everything seems to be working as expected - have left some comments on the specific files where a little bit of cleaning up/documentation is needed.

One additional thing - could you add your extra parameters to the date_comparison() function in comparison_template as well? The typecasting is a really useful feature so it would be great to make the most of it wherever we are using datediffs.

splink/comparison_level_library.py

splink/comparison_library.py

splink/linker.py

RossKen · 2023-03-31T15:45:25Z

splink/spark/spark_base.py

RossKen · 2023-03-31T16:04:34Z

testing_datediff.ipynb

remove from pr

RossKen · 2023-03-31T16:05:01Z

testing_splink_spark.ipynb

remove from pr

RossKen · 2023-03-31T16:05:18Z

tests/.DS_Store

remove from pr

RossKen · 2023-03-31T16:13:11Z

tests/test_datediff_level.py

This makes sense and passes - good work!

Need to be a bit careful here I think: If we need this to make our tests pass, it means others might need it to use Splink. We'd prob rather avoid them needing to change the default if we can avoid it, because it's another thing to document and it might be incompatible with their other (non Splink) code. Was just skimming though so might have got the wrong end of the stick.

The general pattern I've tried to go for elsewhere in splink is to try and use whatever spark config is provided by the user rather than than needing to do specific spark conflict to make splink work

RossKen · 2023-03-31T16:16:58Z

splink/duckdb/duckdb_base.py

instead of doing

if date_format is None: date_format = '%x'

I think it may be better to change the default in cl and cll functions to date_format = '%x' rather than None so it is clearer to the user how dates are being parsed, rather than being hidden in this different function

RossKen · 2023-03-31T16:19:25Z

splink/spark/spark_linker.py

+    # def load_settings(self, settings_dict):
+    #     # call parent method Linker.load_settings
+    #     super().load_settings(settings_dict)
+    #     # if that worked okay then the linker should have `_settings_obj_` and `_settings_dict` set
+    #     # now warn if we need to:
+    #     self._check_ansi_enabled_if_converting_dates()


should these lines be deleted?

RossKen · 2023-03-31T16:28:05Z

Also, per the CI checks - there are a few linting errors. I think they are mostly to do with line length so if you just run source scripts/lint.sh -f # with the fix flag locally (or look in the CI logs) you should be able to find them and fix them pretty easily

Rebaseing datecasting code. SQL working in DuckDb and Spark but need to tweak settings initialization Linker method for Spark

…tion Linker method for Spark

aliceoleary0 · 2023-04-21T14:20:55Z

Please ignore recent activity - didn't realise this would push things through test suite again! Have made the changes above and was trying to resolve some horrible merge conflicts before running the linter

…m:moj-analytical-services/splink into 1030_option_for_auto_typecasting_datediff

…to 1030_option_for_auto_typecasting_datediff

…m:moj-analytical-services/splink into 1030_option_for_auto_typecasting_datediff

RossKen

Thanks for making those changes @aliceoleary0. The examples in the docs and the spark warning have really helped 👍

This is good to merge from my perspective!

ThomasHepworth · 2023-04-25T09:48:10Z

splink/spark/spark_base.py

+    if cast_str:
+        if date_metric == "day":
+            date_f = f"""abs(datediff(to_timestamp({col_name_l},
+            '{date_format}'),to_timestamp({col_name_r},'{date_format}')))"""
+        elif date_metric in ["month", "year"]:
+            date_f = f"""floor(abs(months_between(to_timestamp({col_name_l},
+            '{date_format}'),to_timestamp({col_name_r}, '{date_format}'))"""
+            if date_metric == "year":
+                date_f += " / 12))"
+            else:
+                date_f += "))"
+    else:
+        if date_metric == "day":
+            date_f = f"abs(datediff({col_name_l}, {col_name_r}))"
+        elif date_metric in ["month", "year"]:
+            date_f = f"ceil(abs(months_between({col_name_l}, {col_name_r})"
+            if date_metric == "year":
+                date_f += " / 12))"
+            else:
+                date_f += "))"


Sorry, I've been nosey and had a quick poke around this PR.

I think the following may be slightly cleaner/easier to read:

Suggested change

if cast_str:

if date_metric == "day":

date_f = f"""abs(datediff(to_timestamp({col_name_l},

'{date_format}'),to_timestamp({col_name_r},'{date_format}')))"""

elif date_metric in ["month", "year"]:

date_f = f"""floor(abs(months_between(to_timestamp({col_name_l},

'{date_format}'),to_timestamp({col_name_r}, '{date_format}'))"""

if date_metric == "year":

date_f += " / 12))"

else:

date_f += "))"

else:

if date_metric == "day":

date_f = f"abs(datediff({col_name_l}, {col_name_r}))"

elif date_metric in ["month", "year"]:

date_f = f"ceil(abs(months_between({col_name_l}, {col_name_r})"

if date_metric == "year":

date_f += " / 12))"

else:

date_f += "))"

if cast_str:

col_name_l = f"to_timestamp({col_name_l}, '{date_format}')"

col_name_r = f"to_timestamp({col_name_r}, '{date_format}')"

if date_metric == "day":

date_f = f"abs(datediff({col_name_l}, {col_name_r}))"

elif date_metric in ["month", "year"]:

date_f = f"ceil(abs(months_between({col_name_l}, {col_name_r})"

if date_metric == "year":

date_f += " / 12))"

else:

date_f += "))"

I agree- this is a lot nicer and thanks for the suggestion! How do I integrate this change/ is there a way to reopen the pull request?

Just open a new PR and ask Ross for a quick review 😊

aliceoleary0 added 5 commits March 8, 2023 16:18

SQL working in DuckDb and Spark but need to tweak settings initializa…

16c88e9

…tion Linker method for Spark

Rebaseing datecasting code. SQL working in DuckDb and Spark but need …

207d884

…to tweak settings initialization Linker method for Spark

changes to testing notebooks

9e988b1

fixing testing issues

cfbcb10

merging datediff str casting

c18ef83

aliceoleary0 requested a review from RossKen March 31, 2023 11:09

RossKen reviewed Mar 31, 2023

View reviewed changes

.vscode/launch.json Outdated

Copy link

Contributor

RossKen Mar 31, 2023

Choose a reason for hiding this comment

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

Remove this file

RossKen reviewed Mar 31, 2023

View reviewed changes

.DS_Store Outdated

Copy link

Contributor

RossKen Mar 31, 2023

Choose a reason for hiding this comment

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

Remove this file

RossKen reviewed Mar 31, 2023

View reviewed changes

docs/topic_guides/customising_comparisons.ipynb Outdated Show resolved Hide resolved

RossKen requested changes Mar 31, 2023

View reviewed changes

aliceoleary0 added 9 commits April 21, 2023 11:46

adding docstrings and fixing testing issue

e76ab9a

Rebasing date casting functionality

cc95722

Rebaseing datecasting code. SQL working in DuckDb and Spark but need to tweak settings initialization Linker method for Spark

changes to testing notebooks

b653f97

fixing testing issues

c0cb858

SQL working in DuckDb and Spark but need to tweak settings initializa…

4037270

…tion Linker method for Spark

adding docstrings and fixing testing issue

a24e0aa

rebasing datediff comparisons

8ecec48

rebasing datediff comparisons

330b7c3

lint with black

093dd93

aliceoleary0 added 8 commits April 24, 2023 12:43

adding datediff datecasting to template lib + linting

6289ae1

Merge branch '1030_option_for_auto_typecasting_datediff' of github.co…

5ac7f9a

…m:moj-analytical-services/splink into 1030_option_for_auto_typecasting_datediff

small tweaks to docstrings

0d97748

removing notebooks

fe9da03

removing other files

45f06c0

removing other files

a41c6ca

fixing merge conflicts

ff11af0

Merge branch 'master' of github.com:moj-analytical-services/splink in…

b25d138

…to 1030_option_for_auto_typecasting_datediff

aliceoleary0 added 6 commits April 24, 2023 14:44

lint with black

52b641c

fixing linting issues

90c42ab

removing misc files

bdb2cf6

Merge branch '1030_option_for_auto_typecasting_datediff' of github.co…

7576566

…m:moj-analytical-services/splink into 1030_option_for_auto_typecasting_datediff

removing changes to notebook

11a62af

adjusting error message

d7495b1

aliceoleary0 requested a review from RossKen April 24, 2023 15:28

RossKen approved these changes Apr 24, 2023

View reviewed changes

aliceoleary0 merged commit 3a9e09d into master Apr 25, 2023

ThomasHepworth reviewed Apr 25, 2023

View reviewed changes

RossKen mentioned this pull request Apr 26, 2023

[FEAT] Option for automatic type-casting in datediff_level #1030

Closed

RobinL deleted the 1030_option_for_auto_typecasting_datediff branch August 12, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1030 option for auto typecasting datediff #1162

1030 option for auto typecasting datediff #1162

aliceoleary0 commented Mar 31, 2023

github-actions bot commented Mar 31, 2023 •

edited

Loading

RossKen Mar 31, 2023

RossKen Mar 31, 2023

RossKen left a comment

RossKen Mar 31, 2023

RossKen Mar 31, 2023

RossKen Mar 31, 2023

RossKen Mar 31, 2023

RossKen Mar 31, 2023

RobinL Mar 31, 2023 •

edited

Loading

RossKen Mar 31, 2023 •

edited

Loading

RossKen Mar 31, 2023

RossKen commented Mar 31, 2023

aliceoleary0 commented Apr 21, 2023

RossKen left a comment

ThomasHepworth Apr 25, 2023

aliceoleary0 Apr 25, 2023

ThomasHepworth Apr 25, 2023

1030 option for auto typecasting datediff #1162

1030 option for auto typecasting datediff #1162

Conversation

aliceoleary0 commented Mar 31, 2023

github-actions bot commented Mar 31, 2023 • edited Loading

Test: test_2_rounds_1k_duckdb

Test: test_2_rounds_1k_sqlite

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossKen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinL Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

RossKen Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossKen commented Mar 31, 2023

aliceoleary0 commented Apr 21, 2023

RossKen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 31, 2023 •

edited

Loading

RobinL Mar 31, 2023 •

edited

Loading

RossKen Mar 31, 2023 •

edited

Loading