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

Accumulate Extensions #761

Merged
merged 8 commits into from
Jan 12, 2024
Merged

Conversation

linshokaku
Copy link
Member

@linshokaku linshokaku commented Oct 16, 2023

Add Accumulate Extension for more flexible aggregation of what is captured in the report.

Extensions for the following aggregation methods will be added

  • average
  • min
  • max
  • standard deviation
  • unbiased standard deviation

close #758

@emcastillo emcastillo self-assigned this Oct 17, 2023
@linshokaku linshokaku marked this pull request as ready for review December 21, 2023 11:19
@linshokaku
Copy link
Member Author

/test

@linshokaku
Copy link
Member Author

/test


if self._trigger(manager=manager):
if self._distributed:
summary = self._all_reduce_summaries()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we would like an additional trigger for calling the all_reduce.
Reducing the number of times it is called may improve overall performance but I am not sure if it would be significant.

Copy link
Member Author

Choose a reason for hiding this comment

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

The distributed flag is intended to allow values stored for newly reported keys by this accumulate to be the ones that are all_reduced across processes. If want to control the timing of all_reduce with an independent trigger, it can be difficult for users to understand the meaning of the stored values.

In the current implementation, the number of times all_reduce is performed is minimized within the range satisfying the specifications. Therefore, if want to control the number of all_reduce executions, it can be achieved by adjusting the trigger argument.

emcastillo
emcastillo previously approved these changes Jan 10, 2024
Copy link
Contributor

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM!, some minor nitpicks

@emcastillo
Copy link
Contributor

/test

@emcastillo emcastillo merged commit 859db81 into pfnet:master Jan 12, 2024
17 checks passed
@emcastillo emcastillo added this to the v0.7.6 milestone Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Min/Max in report summaries
2 participants