-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix warnings, update packages, get unit tests to pass again #95
Conversation
…er_group_simple unit test because apparently pandas post-merge row order is no longer guaranteed to be a certain way
…ation_simple unit test because it seems like the order of the merge is based on the c1 column?
@@ -206,10 +206,10 @@ def test_aggregation_simple(): | |||
df3 = model._get_reporting_aggregate_votes(df1, df2, aggregate=["c1", "c2"], estimand=estimand) | |||
assert pd.DataFrame( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are the changes to this unit test necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no guarantee (starting with the most recent version of pandas
, I believe) that the order of the rows in both data frames will be preserved during a merge()
(or join()
). By default they're now being sorted by the columns being merged on, and oddly enough I tried to disable that (with sort=False
) but it made no difference 🤔
But one thing I've been concerned about here as opposed to the test I had to modify in test_gaussian_model.py
....with that test I was able to set the index and compare each value index-by-index. So as a result, the sort order in the merged data frame no longer matters and our unit test should continue to pass. However, I'm not doing that here, so, since the result of this unit test depends on the order of the merged data frame, 😬
I could modify this to set c1
and c2
to be the index and check each results_estimand
and reporting
columns' values that way 🤔 (Another option is https://pandas.pydata.org/docs/reference/api/pandas.testing.assert_frame_equal.html, however, it seems like the index has to match regardless of the row or column order 🤔 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. ok, will approve. but the order not being guarnteed is def something we should remember. It may require changes to the model elsewhere if we are assuming the order (ie alphabetical or by fips code or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Word 😬
Description
Hi! The changes in this PR:
Note that there is still a warning coming from
cvxpy
regarding the fact that inelex-solver
, we haven't explicitly specified which solver to use. That will require a newelex-solver
release at a later date; see ELEX-4413.Jira Ticket
ELEX-3581
Test Steps
tox
🎉