ELEX-4647 more unit tests #145

dmnapolitano · 2024-11-20T14:54:53Z

Description

Hi! The changes in this PR:

~~address pylint complaints, although TODO ones still remain;~~ now resolved in PR Resolve pylint and other warnings #146
~~adds a BaseDataHandler abstract class that most DataHandlers are now subclasses of;~~ now resolved in PR Preprocessed data handler superclass #147
adds some unit tests for VersionedDataHandler although I'm sure there's more scenarios we want to test there though I'm not exactly sure what 🤔
ensures that all top-level imports of external packages are dependencies in setup.py;

Thanks! 🎉

Jira Ticket

ELEX-4647

Test Steps

tox or the elexmodel command of your choice 🎉

lennybronner

What's the purpose of adding the BaseDataHandler?

setup.py

lennybronner · 2024-12-12T15:58:51Z

src/elexmodel/handlers/s3.py

@@ -128,7 +132,7 @@ def wait_for_versions(self, q):
            try:
                future.result()
                yield version, data
-            except Exception as e:
+            except Exception as e:  # pylint: disable=broad-exception-caught


Don't we usually specify this with # noqa rather than pyline?

Hmmmmmm...I think you're right. Let me see what happens if I switch all of these to noqa 🤔

Oh wow, so TIL that pre-commit looks at noqa lines which are part of the PEP8 standard, but pylint doesn't look at noqa at all 🤔 pylint-dev/pylint#2493

So I guess I'll leave these pylint lines in so that pylint doesn't issue these complaints when we run tox 🤔

We didn't really deal with the issues with tox in the past, and I don't think we should moving forward. I think we can stick to just ones that make our pre-commit hooks fail.

dmnapolitano · 2024-12-12T18:20:02Z

What's the purpose of adding the BaseDataHandler?

It's to unify the interface of all of our data handlers as much as possible and reduce some redundancy 🤷🏻‍♀️

lennybronner · 2024-12-12T18:39:42Z

What's the purpose of adding the BaseDataHandler?

It's to unify the interface of all of our data handlers as much as possible and reduce some redundancy 🤷🏻‍♀️

Hmm, I see. I mean the LiveData and the PreprocessedData handler sort of makes sense. but the versioned data handler is very different. In this case the data is results rather then preprocessed data.

src/elexmodel/handlers/data/BaseDataHandler.py

dmnapolitano · 2024-12-12T19:56:06Z

What's the purpose of adding the BaseDataHandler?

It's to unify the interface of all of our data handlers as much as possible and reduce some redundancy 🤷🏻‍♀️

Hmm, I see. I mean the LiveData and the PreprocessedData handler sort of makes sense. but the versioned data handler is very different. In this case the data is results rather then preprocessed data.

Hmmmm, yeah, you're right. There's a bit of overlap but not much and it doesn't really fit with the convention of preprocessed vs. results data. Ok, I'll change it back now 😄

…o getting unit tests to pass again

dmnapolitano · 2024-12-12T20:53:30Z

What's the purpose of adding the BaseDataHandler?

It's to unify the interface of all of our data handlers as much as possible and reduce some redundancy 🤷🏻‍♀️

Hmm, I see. I mean the LiveData and the PreprocessedData handler sort of makes sense. but the versioned data handler is very different. In this case the data is results rather then preprocessed data.

Hmmmm, yeah, you're right. There's a bit of overlap but not much and it doesn't really fit with the convention of preprocessed vs. results data. Ok, I'll change it back now 😄

Done! 🎉

lennybronner · 2024-12-12T22:41:56Z

src/elexmodel/handlers/data/VersionedData.py

-            batch_margin = (
-                np.diff(results_dem, append=results_dem[-1]) - np.diff(results_gop, append=results_gop[-1])
-            ) / np.diff(results_weights, append=results_weights[-1])
+            with warnings.catch_warnings():


where is this coming from?

It happens if you have any zeros in the denominator. Since we probably won't enable this until late into tab anyway, it should be pretty rare, but still, it's nice to not see it in our logs if we know it's a thing we're ok with 🤔

I just realized saying "it happens if you have zeros in the denominator" is not the most useful comment here lol 😅 More like, if you have any results_weights that are still zero due to units that haven't reported, especially in states with a lot of small units (New England, Texas) 🤔

lennybronner · 2024-12-12T22:42:57Z

src/elexmodel/handlers/data/VersionedData.py

@@ -64,10 +65,12 @@ def get_versioned_results(self, filepath=None):

        if self.election_id.startswith("2020-11-03_USA_G"):
            path = "elex-models-prod/2020-general/results/pres/current.csv"
-        elif self.election_id.startswith("2024-11-05_USA_G"):


why has this been removed?

It wasn't, the logic was a little redundant here so I cleaned it up right below this 😄

lennybronner · 2024-12-12T22:43:22Z

src/elexmodel/handlers/data/VersionedData.py

@@ -222,9 +231,8 @@ def get_versioned_predictions(self, filepath=None):
            return pd.read_csv(filepath)

        if self.election_id.startswith("2020-11-03_USA_G"):
-            path = "elex-models-prod/2020-general/prediction/pres/current.csv"


why has this been removed?

Once the exception is raised, path is unused. We don't need it 🤷🏻‍♀️

…d model input data

lennybronner · 2024-12-13T14:58:35Z

I am a little confused by these changes. It's not really what we had discussed. I would pretty strongly prefer to only include the unit tests in this PR (+ the changes to setup.py)

dmnapolitano · 2024-12-13T15:48:37Z

I am a little confused by these changes. It's not really what we had discussed. I would pretty strongly prefer to only include the unit tests in this PR (+ the changes to setup.py)

Hmmm ok, let me see what I can split into separate branches 🤔

dmnapolitano · 2024-12-13T16:57:37Z

I am a little confused by these changes. It's not really what we had discussed. I would pretty strongly prefer to only include the unit tests in this PR (+ the changes to setup.py)

Hmmm ok, let me see what I can split into separate branches 🤔

Done! 🎉

dmnapolitano added 15 commits November 18, 2024 11:00

Addressing some pylint complaints in client.py

5c056e4

Addressing pylint complaint in ConformalElectionModel

9caf865

Addressing pylint complaints in BootstrapElectionModel

ffc8fb3

Addressing some pylint complaints in s3.py

72d3c5e

Addressing some pylint complaints in VersionedData.py

4dece5c

Addressing some pylint complaints in our unit tests

5752f48

Creating a data handler base class

307a95f

Making MockLiveDataHandler a subclass of BaseDataHandler

aa24de8

Correcting variable name in BaseDataHandler

4019fca

Data handlers won't work without self.file_path

d5b6186

Some subclassing / warning-silencing in VersionedDataHandler

addcaed

Resolving a pandas groupby apply warning

1a1725b

Starting to work on some unit tests for the VersionedData

60eda26

Two more versioned data unit tests

a1e07bd

Adding missing cvxpy dependency and sorting dependencies alphabetically

720a6cc

dmnapolitano requested a review from a team as a code owner November 20, 2024 14:54

Fixing a class member name I missed earlier in BaseDataHandler

ec5e072

lennybronner reviewed Dec 12, 2024

View reviewed changes

src/elexmodel/handlers/data/BaseDataHandler.py Outdated Show resolved Hide resolved

Changing self.office to self.office_id in BaseDataHandler and subclasses

f2ca6cf

dmnapolitano added 2 commits December 12, 2024 15:01

Rolling VersionedDataHandler back to what's in develop

8ba8b37

Resolving warnings and pylint complaints in VersionedDataHandler, als…

f122e66

…o getting unit tests to pass again

lennybronner reviewed Dec 12, 2024

View reviewed changes

Renaming BaseDataHandler so that it's more clear it's for preprocesse…

b463e23

…d model input data

dmnapolitano marked this pull request as draft December 13, 2024 15:48

Moving code to fix pylint and other warnings to another branch

da5c31c

dmnapolitano mentioned this pull request Dec 13, 2024

Resolve pylint and other warnings #146

Open

Moving the preprocessed data superclass to a new branch

488f703

dmnapolitano mentioned this pull request Dec 13, 2024

Preprocessed data handler superclass #147

Merged

dmnapolitano marked this pull request as ready for review December 13, 2024 16:56

lennybronner approved these changes Dec 13, 2024

View reviewed changes

dmnapolitano merged commit ed6e9fa into develop Dec 13, 2024
3 checks passed

dmnapolitano deleted the ELEX-4647-more-unit-tests branch December 13, 2024 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ELEX-4647 more unit tests #145

ELEX-4647 more unit tests #145

dmnapolitano commented Nov 20, 2024 •

edited

Loading

lennybronner left a comment

lennybronner Dec 12, 2024

dmnapolitano Dec 12, 2024

dmnapolitano Dec 12, 2024

lennybronner Dec 12, 2024

dmnapolitano commented Dec 12, 2024

lennybronner commented Dec 12, 2024

dmnapolitano commented Dec 12, 2024

dmnapolitano commented Dec 12, 2024

lennybronner Dec 12, 2024

dmnapolitano Dec 13, 2024

dmnapolitano Dec 13, 2024

lennybronner Dec 12, 2024

dmnapolitano Dec 13, 2024

lennybronner Dec 12, 2024

dmnapolitano Dec 13, 2024

lennybronner commented Dec 13, 2024

dmnapolitano commented Dec 13, 2024

dmnapolitano commented Dec 13, 2024

ELEX-4647 more unit tests #145

ELEX-4647 more unit tests #145

Conversation

dmnapolitano commented Nov 20, 2024 • edited Loading

Description

Jira Ticket

Test Steps

lennybronner 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

dmnapolitano commented Dec 12, 2024

lennybronner commented Dec 12, 2024

dmnapolitano commented Dec 12, 2024

dmnapolitano commented Dec 12, 2024

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

Choose a reason for hiding this comment

lennybronner commented Dec 13, 2024

dmnapolitano commented Dec 13, 2024

dmnapolitano commented Dec 13, 2024

dmnapolitano commented Nov 20, 2024 •

edited

Loading