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

Change assert #712

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Change assert #712

merged 2 commits into from
Apr 17, 2020

Conversation

MiryamMV
Copy link
Collaborator

After replacing the _compare_dataframes IC function with the pandas.testing function assert_frame_equal (with @mmkekic help) there are 3 test that fail. These are beersheba_test.py (not clear), esmeralda_test.py (it fails because the index of the compared dataframes is different) and corrections_test.py (not clear to me either, but apparently not related to the changes I made).
@mmkekic suggested to open this PR so we can discuss what approach to take @andLaing @gonzaponte

@mmkekic
Copy link
Collaborator

mmkekic commented Mar 27, 2020

I believe that all the beersheba_test.py::test_create_deconvolution_df and esmeralda_test.py::test_esmeralda_tracks_exact should be fixed by resetting index when comparing, what do you think @gonzaponte ? And based on the Travis I believe those are the only two tests that fail, @MiryamMV the fails were local right?

@gonzaponte
Copy link
Collaborator

If that fixes it, go ahead. I don't think there is a way to ignore the index column in that function.

As for the one in corrections_test, don't worry about it. That is a flaky test that will be removed soon.

@MiryamMV
Copy link
Collaborator Author

I believe that all the beersheba_test.py::test_create_deconvolution_df and esmeralda_test.py::test_esmeralda_tracks_exact should be fixed by resetting index when comparing, what do you think @gonzaponte ? And based on the Travis I believe those are the only two tests that fail, @MiryamMV the fails were local right?

Yes!! The fails were local

@andLaing
Copy link
Collaborator

It appears for me that travis still hasn't finished, is there something wrong with travis?

Copy link
Collaborator

@mmkekic mmkekic left a comment

Choose a reason for hiding this comment

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

I believe this PR should have 2 commits, one where the function in testing_utils are replaced with the pandas one, and other where esmeralda and beersheba test are corrected for different indices. (Basically squash first three commits and give it a meaningful name)

@@ -27,7 +27,9 @@ def test_create_deconvolution_df(ICDATADIR):
CutType.abs, ecut, 3) for _, t in true_dst.groupby('event')])
true_dst = true_dst.loc[true_dst.E > ecut, :].reset_index(drop=True)

assert_dataframes_close(new_dst, true_dst)
#new_dst .reset_index(drop=True)#
#true_dst.reset_index(drop=True)#
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove instead of comment

@@ -163,7 +163,8 @@ def test_esmeralda_tracks_exact(data_hdst, esmeralda_tracks, correction_map_file
#some events are not in df_tracks_exact
events = df_tracks_exact.event.unique()
df_tracks_cut = df_tracks[df_tracks.event.isin(events)]
assert_dataframes_close (df_tracks_cut[columns2], df_tracks_exact[columns2])

assert_dataframes_close (df_tracks_cut[columns2].reset_index(drop=True), df_tracks_exact[columns2].reset_index(drop=True))
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe split the line, is a bit long


def assert_dataframes_close(df1, df2, check_types=True, rtol=None, atol=None, **kwargs):
if rtol:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can someone else also check this makes sense, @gonzaponte , @andLaing ?

Copy link
Collaborator

@mmkekic mmkekic left a comment

Choose a reason for hiding this comment

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

This PR replaces dataframe comparison from the custom to pandas one, that is more suitable when comparing mixed (floats and strings) dataframes. Good job @MiryamMV and welcome to IC developers!

Copy link
Collaborator

@mmkekic mmkekic left a comment

Choose a reason for hiding this comment

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

This PR replaces dataframe comparison from the custom to pandas one, that is more suitable when comparing mixed (floats and strings) dataframes. Good job @MiryamMV and welcome to IC developers!

@bpalmeiro bpalmeiro merged commit 637fc00 into next-exp:master Apr 17, 2020
@MiryamMV MiryamMV deleted the change_assert branch July 14, 2020 15:43
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.

5 participants