Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

fix extended coverage of masking tests #542

Merged
merged 2 commits into from
Oct 2, 2020
Merged

fix extended coverage of masking tests #542

merged 2 commits into from
Oct 2, 2020

Conversation

finiteprods
Copy link
Contributor

@finiteprods finiteprods commented Oct 1, 2020

#524 introduced various changes to tidy the API especially around the generalised scalar extension. As such, the masking tests (several hundred tests generated by the macros test_masking, test_aggregation and test_masking_and_aggregation) now cover the (un)masking and aggregation of the scalar as well.

An issue was discovered in #524 where the unmasked scalar sum appeared larger than expected. This lead to "closeness checks" of model weights to exceed the desired tolerance, and so those tests (at least, those based on test_masking and test_masking_and_aggregation) were temporarily relaxed. This PR contains a fix for that issue, which was due to an inconsistency in the use of the PRNG in masking vs unmasking.

All masking tests are now reinstated, although a related issue has been found with a few test cases of test_masking_and_aggregation. Again, closeness checks break for those cases - for now, a slightly fewer number of models are aggregated to keep the difference within tolerance.

On a separate note, this PR also contains a more complete implementation of MaskObjectBuffer.

@finiteprods finiteprods changed the title extend coverage of masking tests fix extended coverage of masking tests Oct 1, 2020
Copy link
Contributor

@janpetschexain janpetschexain left a comment

Choose a reason for hiding this comment

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

apart from XN-1180 this is good to go, but this shouldn't prevent merging this pr.

two things that might either be added here or in a follow up ticket:

  • the naming should be consistified, currently many/vector/n and one/scalar/1 are used interchangeably in various places
  • the unmasking can be optimized memory wise, currently we do to collect allocations, but if the order of operations is changed to "unmask scalar -> unmask model and apply correction directly in the map -> collect" we can save a whole vector allocation for the model

@finiteprods
Copy link
Contributor Author

Thanks for reviewing @janpetschexain and pointing out the memory optimization. Since there is already a plan to deal with more consistent naming in a PR soon, let's also roll the memory optimization into there.

@finiteprods finiteprods merged commit 3b816eb into master Oct 2, 2020
@finiteprods finiteprods deleted the scalar-tests branch October 2, 2020 13:18
@finiteprods finiteprods mentioned this pull request Feb 4, 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.

2 participants