[stdlib] Implement `Dict.setdefault(key, default)` #2803

msaelices · 2024-05-23T20:45:00Z

Implement Dict.setdefault(key, default)
Tests for Dict.setdefault(key, default)

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

gabrieldemarmiesse

Thank you for the pull request! It's a very useful function to have ! I actually discoved it last week and it made my code much simpler. I think we can improve the performance by avoiding copies and working with references whenever possible.

gabrieldemarmiesse · 2024-05-24T20:05:46Z

stdlib/src/collections/dict.mojo

@@ -855,6 +855,22 @@ struct Dict[K: KeyElement, V: CollectionElement](
        self._index = _DictIndex(self._reserved)
        self._entries = Self._new_entries(self._reserved)

+    fn setdefault(inout self, key: K, owned default: V) -> V:


Suggested change

fn setdefault(inout self, key: K, owned default: V) -> V:

fn setdefault(inout self: Reference[Self, _, _], key: K, owned default: V) -> Reference[V, self.is_mutable, self.lifetime]:

It should be possible to return a reference here to avoid a move. It will make this method work with non-copyable types in the future (and also moar speeeeeed!!!!)

gabrieldemarmiesse · 2024-05-24T20:10:56Z

stdlib/src/collections/dict.mojo

+            self[key] = default
+        return default


The line self[key] = default triggers a copy since there is another use of default afterwards.

What about replacing it with self[key] = default^?

We can then return self.__get_ref(key) to work with references :) No copy there.

I think in the future, for even better performance, we should return a reference to the object being inserted in the Dict._insert function. That can avoid a second lookup. It can be another pull request as it's more complicated.

Thanks for the suggestions. Unfortunately, the self[][key] = default^ trick is not working:

❯ ./stdlib/scripts/run-tests.sh Creating build directory for building the stdlib running the tests in. Packaging up the Standard Library. Included from /home/msaelices/src/mojo/stdlib/src/collections/__init__.mojo:1: /home/msaelices/src/mojo/stdlib/src/collections/dict.mojo:871:19: error: expression must be mutable in assignment self[][key] = default^ ~~~~~~^~~~~ mojo: error: failed to parse the provided Mojo source module

Maybe self should be mutable for this to work?

Ok, the problem is that self needs to be declared as inout. The problem is that if I do that change I am now receiving this error:

maybe fn setdefault(self: Reference[Self, True, _], ...

Also if that's possible, can you avoid using screenshots? If you use copy-paste of text, it's better, as other can copy paste your error messages, and can also search for them from the search bar of github.

I followed your suggestions in this commit, even if it's failing:
msaelices@7b3844f

I'm waiting for your expertise @gabrieldemarmiesse . Thanks in advance.

I tested locally and this is the right fix. It works. Notice the True inside the Reference. Also you need to remove the inout

EDIT: Locally in the unit tests, I had to dereference the result with [], which is expected

See gabrieldemarmiesse@1144383 for the fix

Thanks so much. I've just merged it in this PR: msaelices#1

gabrieldemarmiesse · 2024-05-24T20:15:45Z

stdlib/test/collections/test_dict.mojo

@@ -524,10 +524,21 @@ fn test_clear() raises:
    assert_equal(len(some_dict), 0)


+fn test_dict_setdefault() raises:


Can we add another test that verifies that no copies are happening when calling setdefault ? We can use the CopyCounter struct for this purpose. There is an example here: https://github.com/modularml/mojo/blob/nightly/stdlib/test/collections/test_list.mojo#L568

Will do it when I manage to fix the compiler issue above. Thank you!

There is still this new test to add

Done here: msaelices@7e53b09

It's curious. I thought the case of the "b" key would have 0 copies but it's 1.

Maybe you should use the transfer operator in the test ^? Maybe that's why there is a copy. Also I think calling __getitem__ triggers a copy

I tried it but no luck: msaelices@91d9ee5

It may be a bug in the dict implementation. let's skip this problem for now. I think it needs to be investigated later on, but that would be out of scope for this PR.

gabrieldemarmiesse · 2024-05-24T20:19:57Z

Also can you rename your pull request? By putting [stdlib] at the start, it will make the CI happy :)

@gabrieldemarmiesse

It will not copy while returning the value Suggested by @gabrieldemarmiesse Signed-off-by: Manuel Saelices <msaelices@gmail.com>

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Fix implementation

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

msaelices · 2024-05-26T15:05:30Z

@gabrieldemarmiesse I've merged your fix, and also synced the nightly branch fixing conflicts. Could you please TAL?

Thanks so much.

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

gabrieldemarmiesse · 2024-05-26T17:21:35Z

stdlib/src/collections/dict.mojo

+            self[][key] = default^
+        return self[].__get_ref(key)


Suggested change

self[][key] = default^

return self[].__get_ref(key)

self[][key] = default^

return self[].__get_ref(key)

There is no need to put it outside the except, no? Is it because it fails at compile time otherwise?

No special reason. Fixed here: msaelices@ab36941

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

msaelices · 2024-06-10T21:56:00Z

@gabrieldemarmiesse could you please take another look? I've synced from nightly and fixed the issues because of breaking changes from that branch.

gabrieldemarmiesse

Looks good to me! There is still the problem with the copy to be investiguated, but I believe it's unrelated to your PR so we can skip it for now. At least it's documented in the unit tests :)

gabrieldemarmiesse

Looks good to me! There is still the problem with the copy to be investiguated, but I believe it's unrelated to your PR so we can skip it for now. At least it's documented in the unit tests :)

Signed-off-by: Connor Gray <code@connorgray.com>

ConnorGray · 2024-06-29T20:51:31Z

!sync

modularbot · 2024-07-18T20:34:17Z

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[External] [stdlib] Implement `Dict.setdefault(key, default)` Implement `Dict.setdefault` which returns a reference with the value of the item with the specified key. If the key does not exist, it is inserted with the specified value into the `Dict`. Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes #2803 MODULAR_ORIG_COMMIT_REV_ID: a5514f823c3b7dc978607435d8082f1efe14aa13

modularbot · 2024-07-19T06:23:34Z

Landed in a842307! Thank you for your contribution 🎉

[External] [stdlib] Implement `Dict.setdefault(key, default)` Implement `Dict.setdefault` which returns a reference with the value of the item with the specified key. If the key does not exist, it is inserted with the specified value into the `Dict`. Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes #2803 MODULAR_ORIG_COMMIT_REV_ID: a5514f823c3b7dc978607435d8082f1efe14aa13

msaelices added 2 commits May 23, 2024 22:35

[stdlib] Implement Dict.setdefault()

e6aa1a8

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

[stdlib] Tests for Dict.setdefault()

6d3ed56

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

msaelices requested review from a team as code owners May 23, 2024 20:45

[stdlib] Update changelog.md

e81f4de

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

msaelices force-pushed the dict-setdefault branch from 43bf8d7 to e81f4de Compare May 23, 2024 21:02

gabrieldemarmiesse reviewed May 24, 2024

View reviewed changes

Merge branch 'nightly' into dict-setdefault

f1e5bc8

msaelices changed the title ~~Implement Dict.setdefault(key, default)~~ [stdlib] Implement Dict.setdefault(key, default) May 24, 2024

msaelices and others added 4 commits May 25, 2024 00:51

[stdlib] Use References to speed-up the setdefault() logic

7b3844f

It will not copy while returning the value Suggested by @gabrieldemarmiesse Signed-off-by: Manuel Saelices <msaelices@gmail.com>

Fix implementation

1144383

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Merge pull request #1 from gabrieldemarmiesse/show_fix

dda4b6d

Fix implementation

Merge branch 'nightly' into dict-setdefault

a4fd6a1

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

msaelices requested a review from gabrieldemarmiesse May 26, 2024 15:04

[stdlib] Test that checks the number of copies done by setdefault()

7e53b09

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

gabrieldemarmiesse reviewed May 26, 2024

View reviewed changes

msaelices requested a review from gabrieldemarmiesse May 26, 2024 21:46

msaelices added 4 commits May 27, 2024 00:14

[stdlib] Use the transfer operator to make sure there are no copies

91d9ee5

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

Merge branch 'nightly' into dict-setdefault

b9f8d4f

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

[stdlib] Fix the code to work in latest Mojo

f2b4127

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

[stdlib] Align the return sentence to the except block

ab36941

Signed-off-by: Manuel Saelices <msaelices@gmail.com>

Merge branch 'nightly' into dict-setdefault

3450316

gabrieldemarmiesse approved these changes Jun 11, 2024

View reviewed changes

msaelices added 3 commits June 12, 2024 09:31

Merge branch 'nightly' into dict-setdefault

724daf8

Merge branch 'nightly' into dict-setdefault

eb13caa

Merge branch 'nightly' into dict-setdefault

3c989ab

msaelices and others added 2 commits June 25, 2024 22:41

Merge branch 'nightly' into dict-setdefault

607b263

Merge branch 'nightly' into dict-setdefault

3adb90c

Signed-off-by: Connor Gray <code@connorgray.com>

modular-automation bot assigned ConnorGray Jun 29, 2024

modularbot added the imported-internally Signals that a given pull request has been imported internally. label Jun 29, 2024

modularbot added the merged-internally Indicates that this pull request has been merged internally label Jul 18, 2024

modularbot added the merged-externally Merged externally in public mojo repo label Jul 19, 2024

modularbot closed this Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] Implement `Dict.setdefault(key, default)` #2803

[stdlib] Implement `Dict.setdefault(key, default)` #2803

msaelices commented May 23, 2024

gabrieldemarmiesse left a comment

gabrieldemarmiesse May 24, 2024 •

edited

Loading

gabrieldemarmiesse May 24, 2024

msaelices May 24, 2024

gabrieldemarmiesse May 24, 2024

msaelices May 24, 2024

gabrieldemarmiesse May 24, 2024

msaelices May 24, 2024

gabrieldemarmiesse May 24, 2024 •

edited

Loading

gabrieldemarmiesse May 25, 2024

msaelices May 26, 2024

gabrieldemarmiesse May 24, 2024

msaelices May 24, 2024

gabrieldemarmiesse May 26, 2024

msaelices May 26, 2024

gabrieldemarmiesse May 26, 2024

msaelices May 26, 2024

gabrieldemarmiesse Jun 11, 2024

gabrieldemarmiesse commented May 24, 2024

msaelices commented May 26, 2024

gabrieldemarmiesse May 26, 2024

msaelices Jun 10, 2024

msaelices commented Jun 10, 2024

gabrieldemarmiesse left a comment

gabrieldemarmiesse left a comment

ConnorGray commented Jun 29, 2024

modularbot commented Jul 18, 2024

modularbot commented Jul 19, 2024

	fn setdefault(inout self, key: K, owned default: V) -> V:
	fn setdefault(inout self: Reference[Self, _, _], key: K, owned default: V) -> Reference[V, self.is_mutable, self.lifetime]:

		@@ -524,10 +524,21 @@ fn test_clear() raises:
		assert_equal(len(some_dict), 0)


		fn test_dict_setdefault() raises:

[stdlib] Implement Dict.setdefault(key, default) #2803

[stdlib] Implement Dict.setdefault(key, default) #2803

Conversation

msaelices commented May 23, 2024

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

gabrieldemarmiesse May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldemarmiesse May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldemarmiesse commented May 24, 2024

msaelices commented May 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msaelices commented Jun 10, 2024

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

ConnorGray commented Jun 29, 2024

modularbot commented Jul 18, 2024

modularbot commented Jul 19, 2024

[stdlib] Implement `Dict.setdefault(key, default)` #2803

[stdlib] Implement `Dict.setdefault(key, default)` #2803

gabrieldemarmiesse May 24, 2024 •

edited

Loading

gabrieldemarmiesse May 24, 2024 •

edited

Loading