BiomedSciAI / fuse-med-ml

A python framework accelerating ML based discovery in the medical field by encouraging code reuse. Batteries included :)
Apache License 2.0
137 stars 34 forks source link

`NDict` merge function overrides values #187

Closed SagiPolaczek closed 2 years ago

SagiPolaczek commented 2 years ago

Describe the bug\ When calling the ndict.merge(other_ndict) when ndict and other_ndict are NDict objects with the same keys, the values of other_dict are overriding ndict's.

If you are not familiar with the NDict object, please visit read ndict docu.

FuseMedML version\ version 0.2.4

Python version\ 3.8.13

To reproduce

from fuse.utils.ndict import NDict

A = NDict({"a": 0})
B = NDict({"a":1})

A.merge(B)
# expected: {'a': [0, 1]} (?) OR print a warning of the overriding
# but we get: {'a': 1}

Expected behavior\ Merge values of the same keys into a sequence (list) OR Print a warning of the overriding

SagiPolaczek commented 2 years ago

@mosheraboh Found this issue. What do you think is the right behavior? Merging values into sequences or print warning?

In my opinion we should merge + print a warning (and add a flag to turn it off). I encountered this issue while examine to merge two batch_dicts..

mosheraboh commented 2 years ago

I think that we should follow the standard python dicts - so override should be ok. Here is a link for reference: https://www.geeksforgeeks.org/merging-and-updating-dictionary-operators-in-python-3-9/ @SagiPolaczek , let me know if it makes sense to you.

SagiPolaczek commented 2 years ago

I think that we should follow the standard python dicts - so override should be ok. Here is a link for reference: https://www.geeksforgeeks.org/merging-and-updating-dictionary-operators-in-python-3-9/ @SagiPolaczek , let me know if it makes sense to you.

Looks OK to me :) I'm closing this issue and leaving it for future ref