-
Notifications
You must be signed in to change notification settings - Fork 228
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
[Feature] Enabled un-seen clients case to check the participation generalization gap #100
Conversation
…upload/download bytes, convergence rounds and time
875008f
to
88ad802
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please see the inline comments for minor suggestions. And I am not sure that whether setting unseen_clients
would affect the behaviors of broadcasting to all neighbors in some applications.
federatedscope/core/fed_runner.py
Outdated
self.server = self._setup_server() | ||
self.server.unseen_clients_id = unseen_clients_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we have better not modify the attributes of server/client in fed_runner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it into the setup_server() as sub action of init
federatedscope/core/fed_runner.py
Outdated
@@ -102,6 +115,8 @@ def _setup_for_standalone(self): | |||
for client_id in range(1, self.cfg.federate.client_num + 1): | |||
self.client[client_id] = self._setup_client( | |||
client_id=client_id, client_model=self._shared_client_model) | |||
if client_id in unseen_clients_id: | |||
self.client[client_id].is_unseen_client = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it into the setup_server() as sub action of init
federatedscope/core/sampler.py
Outdated
self.client_state[indices] = 1 | ||
elif state == 'working': | ||
self.client_state[indices] = 0 | ||
if not isinstance(indices, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we cover np.array here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified accordingly, besides, we used another state named "unseen"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Enabled un-seen clients simulation, such that we can check the participation generalization gap, refer ICLR'22, What Do We Mean by Generalization in Federated Learning?
This PR is based on the another PR (enhanced monitor #90)