search

causify-ai / kaizenflow

KaizenFlow is a framework for Bayesian reasoning and AI/ML stream computing
GNU General Public License v3.0
112 stars 77 forks source link

SorrTask1075_Unit_test_hpandas_dassert_valid_remap #1080

Closed smitpatel49 closed 3 months ago

smitpatel49 commented 4 months ago

1075

smitpatel49 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

smitpatel49 commented 4 months ago

@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",
        }
smitpatel49 commented 4 months ago

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

smitpatel49 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.