-
Notifications
You must be signed in to change notification settings - Fork 28
Conversation
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.
The code changes look good to me. The message parsing tests are going to be annoying to fix, let me know if you need help with that.
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
==========================================
+ Coverage 54.37% 55.61% +1.24%
==========================================
Files 67 67
Lines 3187 3231 +44
==========================================
+ Hits 1733 1797 +64
+ Misses 1454 1434 -20
Continue to review full report at Codecov.
|
df294d6
to
0bd8ad1
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.
Looks good to me👍
I think it makes sense if we have a ticket to remove the expected_participants
field in our code base so that we don't forget this
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.
Very nice. Let's squash or split this into a few meaningful commits before merging though.
indeed, added this additional task to the follow-up story. |
===================================================================== update msg payload from bytes with masked scalar masked scalar to bytes scalar aggregation in state machine update phase pass scalar agg from update to sum2 phase derive scalar mask from seed; aggregate scalar masks add aggregate scalar mask to sum2 message payload rename aggregation to model_agg and scalar_agg collect scalar mask sums
================================================= pass scalar pair sum2 -> unmask; rename mask_dict add scalar agg & mask dict to unmask phase correction step rename agg fields
remaining test fixes
fmt float literal; docstring for correction fn
8c95f95
to
cb66c7d
Compare
Currently, the PET protocol requires that model scalars be in the range
[0,1]
and that they are essentially public data (in particular, known by the coordinator). This merge request extends the protocol to deal with private scalars, local to participants in a similar way that local models are. In addition, the extension generalises scalars to be arbitrarily large i.e.[0, infinity)
.The changes are summarised as follows.
update
message. The message payload now additionally contains a masked scalar.sum
participants.sum2
message. The message payload now additionally contains an aggregated scalar mask.