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

docs(framework) Add examples to records docstrings #4021

Merged
merged 9 commits into from
Aug 21, 2024

Conversation

jafermarq
Copy link
Contributor

@jafermarq jafermarq commented Aug 15, 2024

Updated docstrings for ConfigsRecords, MetricsRecords and ParametersRecords showing examples.

The existing docstrings have been moved out of the __init__() since otherwise they were not shown in the docs.

Not entirely sure if using docs(framework) is a correct tag

@jafermarq jafermarq added the documentation Improvements or additions to documentation label Aug 15, 2024
@jafermarq jafermarq requested a review from panh99 August 15, 2024 16:55
src/py/flwr/common/record/configsrecord.py Outdated Show resolved Hide resolved
src/py/flwr/common/record/metricsrecord.py Outdated Show resolved Hide resolved
src/py/flwr/common/record/configsrecord.py Show resolved Hide resolved
src/py/flwr/common/record/metricsrecord.py Outdated Show resolved Hide resolved
src/py/flwr/common/record/parametersrecord.py Outdated Show resolved Hide resolved
src/py/flwr/common/record/parametersrecord.py Outdated Show resolved Hide resolved
Copy link
Contributor

@panh99 panh99 left a comment

Choose a reason for hiding this comment

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

The PR looks good! Just a few small things to improve.

FYI, both our TypedDict and built-in dict inherit the MutableMapping class, and thus we are safe to claim that TypedDict/*Record are just python dictionaries that users are very familiar with. This claim may help users get start with our records easilier.

Comment on lines 80 to 82
The usage of a :code:`ConfigsRecord` is envisioned for storing Python built-in
types such as :code:`int`, :code:`float`, :code:`str`, :code:`bytes` and lists of
these. All types allowed are defined in
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to not only tell the user what they can technically do with ConfigsRecord, but what it is intended for. It's intended to send configuration values to the client, along with the model. Perhaps we can give a few examples, like sending the number of local epochs the client should perform? Or time-related things?

Comment on lines 79 to 82
The usage of a :code:`MetricsRecord` is envisioned for single scalar
(:code:`int`, :code:`float`) or list of scalars. A :code:`MetricsRecord` is
one of the types of records that a :code:`common.RecordSet` supports.
Let's see some examples:
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for ConfigsRecord

@jafermarq jafermarq marked this pull request as ready for review August 21, 2024 18:07
@danieljanes danieljanes merged commit 53176af into main Aug 21, 2024
33 checks passed
@danieljanes danieljanes deleted the records-docstrings-examples branch August 21, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants