Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

[Retiarii] Nest ValueChoice in LayerChoice and dict/list in ValueChoice #3508

Merged
merged 7 commits into from
Apr 9, 2021

Conversation

ultmaster
Copy link
Contributor

No description provided.


def __getitem__(self, item):
access = copy.deepcopy(self)
access._accessor.append(item)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use append?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying implementation is to clone the current instance, and append item to "accessor", which records all the history getitem calls. For example, when accessor is [a, b, c], the value choice will return vc[a][b][c] where vc is the original value choice.

Copy link
Contributor

@QuanluZhang QuanluZhang Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, very fancy, supporting multi-level indices. but does choice in valuechoice support multi-level choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I checked that. Will throw an exception in case the multi-level choice on any of the candidates does not work.

target = model.get_node_by_name(node.name)
target.update_operation(target.operation.type, {**target.operation.parameters, argname: chosen})
target.update_operation(target.operation.type, {**target.operation.parameters, argname: chosen_value})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems you deal with indices of ValueChoice when ValueChoice is used as initialization parameters. what if the indices are used in forward function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find this case in the tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in test you uses valuechoice as a function's parameter. What if it is used naively? for example, param = vc[0] + 0.1? does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added. It works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, lihaile!

@@ -557,7 +547,8 @@ def _handle_inputchoice(self, module):
def _handle_valuechoice(self, module):
return {
'candidates': module.candidates,
'label': module.label
'label': module.label,
'accessor': module._accessor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the idea of accessor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to newly-added comments and response to Quanlu.

@ultmaster ultmaster changed the title [Retiarii] Supported nested ValueChoice in LayerChoice and dict/list in ValueChoice [Retiarii] Nest ValueChoice in LayerChoice and dict/list in ValueChoice Apr 6, 2021
@SparkSnail SparkSnail merged commit 6808708 into microsoft:master Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants