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

[Python] Python Dictionary remove_from_dict does not work with tuple as dictionary key. #3615

Closed
Freymaurer opened this issue Nov 27, 2023 · 3 comments · Fixed by #3621
Closed

Comments

@Freymaurer
Copy link

Assume the following:

#r "nuget: Fable.Core, 4.2.0"

open System.Collections.Generic

let my_dict_min = Dictionary((Map [for i in 0..10 do yield i, sprintf "Number: %i" i]))

let my_dict = Dictionary((Map [for i in 0..10 do yield (i,i), sprintf "Number: %i" i]))

Getting values from key will work for both dictionary, but when i try to remove values by key it will throw an error for tuple as key.

I checked the transpiled code and saw that my_dict.Remove((0,0)) gets transpiled to: remove_from_dict(my_dict, (0, 0)).

Then checking the fable_library source files we find:

def remove_from_dict(di: dict[_K, Any], k: _K) -> bool:
    if k in di:
        del di[k]
        return True

    return False

I copied the dictionary creation code to a jupyter notebook and tested the del part of remove_from_dict and it turns out that it fails as soon as del is called with a tuple as key for the dictionary

image

As Fable.Python is pretty "cutting edge" i will provide the f# source code without a REPL. It was transpiled using dotnet tool fable 4.6.0

// source code
#r "nuget: Fable.Core, 4.2.0"

open System.Collections.Generic

let my_dict_min = Dictionary((Map [for i in 0..10 do yield i, sprintf "Number: %i" i]))

let my_dict = Dictionary((Map [for i in 0..10 do yield (i,i), sprintf "Number: %i" i]))

let v = my_dict.[0,0] // This will run

printfn "GET: %A" v

my_dict_min.Remove(0) // This will run
my_dict.Remove((0,0)) // This will fail

let removeEven() =
  let mutable rmvKeys = ResizeArray()
  for KeyValue((i1,i2),v) in my_dict do
      // Remove cells of column
      if i1%2 = 0 then
          rmvKeys.Add((i1,i2)) // add to seperate collection first. Fable.Python does not like altering dictionary while iterating over it.
  printfn "END ADD LIST"
  for key in rmvKeys do
      my_dict.Remove(key) |> ignore

removeEven()

printfn "%A" my_dict

@HLWeil

@andreaschrader
Copy link

Del removes a referencefor a namespace. The reference needs to be there.

I guess the reason is that the tuple is not present as a key int he dictionary.
If you use your_dict.pop(your_tuple_key), this should not be a problem.
Does this work?

@MangelMaxime
Copy link
Member

@andreaschrader I tried your solution, but it doesn't work

raceback (most recent call last):
  File "/workspaces/Fable/src/Fable.Cli/../quicktest-py/quicktest.py", line 45, in <module>
    main(sys.argv[1:])
  File "/workspaces/Fable/src/Fable.Cli/../quicktest-py/quicktest.py", line 39, in main
    ignore(remove_from_dict(dic_1, (1, 2)))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/Fable/src/quicktest-py/fable_modules/fable_library/map_util.py", line 81, in remove_from_dict
    di.pop(k)
    ^^^^^^
AttributeError: 'Dictionary' object has no attribute 'pop'

Del removes a referencefor a namespace. The reference needs to be there.

I am not exactly sure what you mean by that, but del is called only if the key is present in the dictionary.

def remove_from_dict(di: dict[_K, Any], k: _K) -> bool:
    if k in di:
        del di[k]
        return True

    return False

@dbrattli
Copy link
Collaborator

The problem here is that the Dictionary class is transpiled from F# so it doesn't currently implement __delitem__ for remove. I'll make a fix for this.

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 a pull request may close this issue.

4 participants