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

migrate ComparisonProperties #1195

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

ThomasHepworth
Copy link
Contributor

Preface

I've been having a quick play around with the Comparison levels and seeing if there's anything we can quickly do to clean up the workflow. At the moment it's rather convoluted and difficult to follow.

The problem is, the inheritance structure we currently have doesn't allow much flexibility for moving everything about. It's also quite clear once you get to the individual dialect stage.

The proposed chart diagrams should really help here and I think it's worthwhile getting those right.


Changes

Anyway... this PR simply shifts the location of the ComparisonProperties class.

I'd propose we do this for two reasons:

  1. We no longer import _level comparisons when importing the _comparison_library.py files
  2. It feels a more natural fit for the class. You create the subclasses within that folder and then bind them onto the ComparisonProperties class.

If you disagree or have a differing view, feel free to shout. I can close this PR and we can have a chat.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 21, 2023

Test: test_2_rounds_1k_duckdb

Percentage change: -29.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
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
1581 2023-04-24 11:08:49 1.34152 1.32128 (detached head) 8ce2201 Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz 2.5939 GHz 8ce2201

Test: test_2_rounds_1k_sqlite

Percentage change: -24.1%

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
1583 2023-04-24 11:08:49 3.23515 3.23138 (detached head) 8ce2201 Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz 2.5939 GHz 8ce2201

Click here for vega lite time series charts

Copy link
Contributor

@ADBond ADBond left a comment

Choose a reason for hiding this comment

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

Yep I think this makes sense, seems like a sensible change - certainly the cleaner imports suggest this is a more sensible structure.

@ThomasHepworth ThomasHepworth merged commit bdcf0b0 into master Apr 24, 2023
@ThomasHepworth ThomasHepworth deleted the migrate_comparison_properties branch April 24, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants