Updated ensmallen_graph causes panic in some unit tests

justaddcoffee commented 4 years ago

Describe the bug

In TestEdges.test_make_edges_pos_train_test_valid_edges_distinct_1_neg_train_edges_tsv, I'm seeing a panic:

        # make negative edges
        logging.info("Making negative edges")

        all_negative_edges = \
            pos_train_edges.sample_negatives(seed=seed,
                                             negatives_number=graph.get_edges_number(),
                                             allow_selfloops=False)
        neg_train_edges, neg_test_edges = \
>           all_negative_edges.random_holdout(seed=seed, train_percentage=train_fraction)
E       pyo3_runtime.PanicException: index out of bounds: the len is 0 but the index is 53

To Reproduce

pip install ensmallen_graph -U pytest test

Expected behavior

Should pass holdout unit tests

Version

f012b45f8df0badcda4e3f40836acd082ac97076

Additional context

Also causes problems when loading graphs because my unit tests are using deprecated args in from_csv (force_conversion_to_undirected, possibly others)

LucaCappelletti94 commented 4 years ago

On it!

LucaCappelletti94 commented 4 years ago

Reproduced error, working on fixing it.

LucaCappelletti94 commented 4 years ago

Fixed, I will close this issue when we will publish the new version. The error was caused by leaving the edge types mapping within the graph with negative edges, which cannot have edge types by definition.

justaddcoffee commented 4 years ago

Excellent, thanks @LucaCappelletti94 ! Ping me when the new version is published and I'll unpin this dependency

LucaCappelletti94 commented 4 years ago

Does this pass now?

justaddcoffee commented 4 years ago

Yep, merged now - thanks @LucaCappelletti94

Knowledge-Graph-Hub / kg-covid-19