SorrTask1075_Unit_test_hpandas_dassert_valid_remap

[smitpatel49]

#1075

[smitpatel49]

Can you please give me suggestions on how to improve on this one @sonaalKant, @samarth9008 mainly test3 and test4 as I was expecting an error raise but it went through. Also, what other input variations could be made, especially with to_remap for rows / columns remap.
Thank you.

[samarth9008]

Can you show the error while running test 3 and 4?

Would be nice if you can explain what condition you are trying to check in those tests.

[samarth9008]

Also, what other input variations could be made, especially with to_remap for rows / columns remap.

I would check all the conditions, assertions where the function would break and check those conditions.

[gpsaggese]

LGTM. One comment about checking the PR after the linter

[smitpatel49]

@samarth9008, I just wanted to make sure that my method in test 3 was correct and I didn't have any error in test4, but I was curious regarding the input as it had duplicate keys in the dictionary and was expecting some error.

remap_dict = {
            "dummy_value_1": "1, 2, 3",
            "dummy_value_2": "A, B, C",
            "dummy_value_1": "A1, B2, C3",
        }

[gpsaggese]

LGTM

[samarth9008]

Lets add tests for this 2 lines as well

hdbg.dassert_isinstance(to_remap, list)
hdbg.dassert_isinstance(remap_dict, dict)

[smitpatel49]

Awaiting further review/ instructions on this one. @samarth9008, @gpsaggese, @sonaalKant , @DanilYachmenev

[samarth9008]

Some nits from my side.

[samarth9008]

Did we add check for this LOC - hdbg.dassert_not_intersection(remap_dict.values(), to_remap)

[smitpatel49]

That was my initial question regarding the input, the function mentions row/ columns to map, the to_remap list will contain columns as in keys but how to include values as when calling hdbg.dassert_is_subset we take the list as whole - just containing keys.

[gpsaggese]

What's the next step here? IMO it's best to merge and do a follow up PR. We don't want PRs on deck for 2 weeks. A PR should be merged in 1-2 days max.

[smitpatel49]

I think that would be appropriate. I have been waiting on review from others and active response for a very long time on this PR.

[samarth9008]

LG merging.

[samarth9008]

We don't want PRs on deck for 2 weeks. A PR should be merged in 1-2 days max.

The blockage was from my side as I have multiple things going on so couldn't find time to review these PRs.