Added SUM as aggregation type for custom statistics #4816

brccabral · 2021-01-03T05:37:26Z

Added SUM as aggregation type for custom statistics

chriselion

Overall, I think this is a good change and I can see how it would be useful. I have some feedback on the particular implementation though. There are two (mostly mutually exclusive) ways to go from here:

Option 1

Instead of renaming StatsSummary.mean, you could add a sum field, and also add an aggregation_method field. Then in the StatsWriter implementation (for example

ml-agents/ml-agents/mlagents/trainers/stats.py

Line 173 in f91212b

self.summary_writers[category].add_scalar(f"{key}", value.mean, step)

) you can use aggregation_method to decide whether to use mean or sum.

You could also add an aggregated_value property to StatsSummary like

@property
def aggregated_value(self):
  return self.sum if aggregated_value == StatsAggregationMethod.SUM else self.mean

Option 2

Add a method to StatsReporter like increment_stat, and call that from record_environment_stats() when agg_type == SUM. That would look something like

    def increment_stat(self, key: str, value: float) -> None:
        with StatsReporter.lock:
            if StatsReporter.stats_dict[self.category][key]:
                # Add to the last stat. If we're always using increment_stat, this
                # should be the only element in the list. 
                StatsReporter.stats_dict[self.category][key][-1] += value
            else:
                # New list with the value as the only element
                StatsReporter.stats_dict[self.category][key] = [value]

(but make sure you test it 😃 )

I think I prefer the second approach because it's more consistent with the existing idea of "aggregating" in the StatsReporter and it doesn't require storing the AggregationMethod, but the first approach is more flexible (especially since we're planning to let users add their own StatsWriters soon, see the plugins PR).

Do you have a preference for either one?

chriselion · 2021-01-06T02:27:21Z

ml-agents-envs/mlagents_envs/side_channel/stats_side_channel.py

@@ -35,6 +38,7 @@ def __init__(self) -> None:
    def on_message_received(self, msg: IncomingMessage) -> None:
        """
        Receive the message from the environment, and save it for later retrieval.
+


Please remove the extra whitespaces that were added in the docstrings.

the extra spaces are necessary for PyCharm

Please try to find the settings to disable them; I use PyCharm as do several other team members, and this hasn't been a problem before. At a minimum, you need to revert files that only contain the whitespace changes:

ml-agents/mlagents/trainers/ppo/trainer.py

ml-agents/mlagents/trainers/torch/components/reward_providers/gail_reward_provider.py

ml-agents/mlagents/trainers/trainer/rl_trainer.py

On the other hand, if you know of a way to automate this from the command line (and enforce that the :param ... names match the args), I'd love that in another PR.

chriselion · 2021-01-06T02:28:37Z

ml-agents/mlagents/trainers/tests/test_agent_processor.py

@@ -272,8 +272,8 @@ def test_agent_manager_stats():
        manager.record_environment_stats(env_stats, worker_id=0)

    expected_stats = {
-        "averaged": StatsSummary(mean=2.0, std=mock.ANY, num=2),
-        "most_recent": StatsSummary(mean=4.0, std=0.0, num=1),
+        "averaged": StatsSummary(stats_value=2.0, std=mock.ANY, num=2),


You should also extend this test to include StatsAggregationMethod.SUM

New commit to extend it

chriselion · 2021-01-06T02:30:16Z

ml-agents/mlagents/trainers/stats.py

            log_info.append(f"Std of Reward: {stats_summary.std:0.3f}")
+            log_info.append(f"Num of Reward: {stats_summary.num:0.3f}")


I don't think you should add this; it's not useful for training.

For me it was. If you have a Decision Request script attached, the expected value may change.
In my case, MaxStep = 3000, DecisionRequest = 6, but I wasn't sure how this works at that time.
My custom stat was 0.3 every step.
0.3 * 3000 = 900
but instead I was getting 150, and I didn't know why.
Only after I added the "Num of Reward" I noticed that I was getting 500 Num.
That is because 3000 / 6 = 500.
So,
0.3 * 3000 / 6 = 150.
Because I didn't know how DecisionRequest worked, it took me a while to figure this out.
With the "Num of Rewards" I think it would have saved me a few hours.
Well, that was my experience, I won't mind if you want to remove it

Yeah, I'd like you to remove it. You'll be able to add your own StatsWriter that gets called soon.

I removed this (and updated the tests)

chriselion · 2021-01-06T02:31:41Z

ml-agents/mlagents/trainers/tests/test_learn.py

@@ -75,17 +76,24 @@ def test_run_training(
                learn.run_training(0, options)
                mock_init.assert_called_once_with(
                    trainer_factory_mock.return_value,
-                    "results/ppo",
+                    os.path.join("results", "ppo"),


I don't think this is a bad change, but does it need to happen in this PR? Does this test fail on a certain platform (Windows?) without it?

Yes, it fails in Windows (I am using Windows)

OK. I'll make a note for us to run pytest on Windows too.

brccabral · 2021-01-06T04:01:02Z

Hi @chriselion
In my commit "b7328ba" I did just as you mentioned in Option 1.

class StatsSummary(NamedTuple):
    mean: float
    std: float
    sum: float
    num: int
    aggregation: StatsAggregationMethod

    @staticmethod
    def empty() -> "StatsSummary":
        return StatsSummary(0.0, 0.0, 0.0, 0, StatsAggregationMethod.AVERAGE)

but I would have to rewrite all the writers classes (ConsoleWriter, GaugeWriter, TensorboardWriter) as they all have their own write_stats() method.
By renaming it to "stats_value", all three classes get the implementation without major changes.
Also, if the aggregation is SUM, there is no need to store AVG data in stats_dict[].
And, in the future, I could use other aggregations, MAX/MIN/PERCENTILE.
Would the class StatsSummary have more and more fields?

Option 2 would not give much flexibility to add other aggregations, we are writing our own implementations, but numpy already have them implemented.

…have sum

ml-agents/mlagents/trainers/stats.py

chriselion · 2021-01-06T23:52:30Z

I would have to rewrite all the writers classes (ConsoleWriter, GaugeWriter, TensorboardWriter) as they all have their own write_stats() method.

If you add the property I suggested, you wouldn't need to add any extra logic in the writers. You could even call the property stats_value.

Also, if the aggregation is SUM, there is no need to store AVG data in stats_dict[].

Maybe, but I'd rather not make the logic any more complicated.

And, in the future, I could use other aggregations, MAX/MIN/PERCENTILE. Would the class StatsSummary have more and more fields?

We can add them as we need them.

chriselion · 2021-01-06T23:57:38Z

ml-agents/mlagents/trainers/stats.py

-                std=np.std(StatsReporter.stats_dict[self.category][key]),
-                num=len(StatsReporter.stats_dict[self.category][key]),
-            )
+            if (


It's not your fault, but this method is getting messy and should be cleaned up. Something like

stat_values = StatsReporter.stats_dict[self.category][key] if len(stat_values) == 0: return StatsSummary.empty() return StatsSummary( mean=np.mean(stat_values), sum=np.sum(stat_values), std=np.std(stat_values), num=len(stat_values), aggregation_method = StatsReporter.stats_aggregation[self.category][key], )

should be a bit cleaner.

brccabral · 2021-01-07T07:23:18Z

Hi @chriselion ,
I did the changes you requested, except the docstring, someone already have a tracking issue in jetbrains
https://youtrack.jetbrains.com/issue/PY-26281

This is what happens in my case:

chriselion · 2021-01-08T01:21:55Z

ml-agents/mlagents/trainers/stats.py

@@ -95,8 +108,8 @@ def write_stats(
    ) -> None:
        for val, stats_summary in values.items():
            set_gauge(
-                GaugeWriter.sanitize_string(f"{category}.{val}.mean"),
-                float(stats_summary.mean),
+                GaugeWriter.sanitize_string(f"{category}.{val}.aggregated_value"),


This is going to break some of our internal processes (that live outside of this repo).

Can you make it:

set_gauge(GaugeWriter.sanitize_string(f"{category}.{val}.mean"), float(stats_summary.mean)) set_gauge(GaugeWriter.sanitize_string(f"{category}.{val}.sum"), float(stats_summary.sum))

instead?

ml-agents/mlagents/trainers/stats.py

ml-agents/mlagents/trainers/trainer/rl_trainer.py

ml-agents/mlagents/trainers/torch/components/reward_providers/gail_reward_provider.py

Project/Assets/ML-Agents/Examples/Hallway/Scripts/HallwayAgent.cs

chriselion · 2021-01-08T01:41:45Z

Thanks, I think it's almost there! I left a few final comments but otherwise it looks pretty good.

Sorry to keep harping on the newlines, but I don't think a bug in pycharm's display is a worthwhile reason to change. I'm OK with them in the files that you have other changes in, but files where those are the only changes should be reverted (I can do this for you in git, as long as you have the setting on the PR to allow repo owners to push changes).

brccabral · 2021-01-08T03:59:26Z

All done, @chriselion

chriselion · 2021-01-08T18:00:08Z

ml-agents/mlagents/trainers/agent_processor.py

+        self,
+        step: Union[
+            TerminalStep, DecisionStep
+        ],  # pylint: disable=unsubscriptable-object


See pylint-dev/pylint#2377 (sigh)

chriselion · 2021-01-08T18:00:24Z

ml-agents/mlagents/trainers/stats.py

@@ -37,14 +40,23 @@ def _dict_to_str(param_dict: Dict[str, Any], num_tabs: int) -> str:
        )


-class StatsSummary(NamedTuple):
+class StatsSummary(NamedTuple):  # pylint: disable=inherit-non-class


See pylint-dev/pylint#3876 (sigh)

chriselion · 2021-01-08T18:05:01Z

Thanks, I made a few small cleanups and reverts. Will merge this when tests pass.

brccabral added 10 commits December 25, 2020 10:58

remove Builds folder from git

953b08c

log number of rewards in cmd summary

38afbde

added sum to StatsSummary

b9a18a0

added SUM in Unity StatAggregationMethod

9c3f059

added support for SUM as StatsAggregationMethod in python mlagents

b7328ba

example to use SUM as aggregation

f178316

fixed field order with default values for StatsSummary

e7c45cd

simplified StatsSummary

b6c9a2e

add default value for custom stats

9206b46

fixed tests

4a44514

brccabral changed the title ~~log number of rewards in cmd summary~~ Added SUM as aggregation type for custom statistics Jan 3, 2021

chriselion self-requested a review January 6, 2021 01:51

chriselion self-assigned this Jan 6, 2021

chriselion suggested changes Jan 6, 2021

View reviewed changes

chriselion reviewed Jan 6, 2021

View reviewed changes

extended test test_agent_manager_stats in test_agent_processor.py to …

1299062

…have sum

chriselion reviewed Jan 6, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Show resolved Hide resolved

chriselion reviewed Jan 6, 2021

View reviewed changes

brccabral added 2 commits January 6, 2021 22:51

refractor StatsSummary to add sum as property

f667c0b

fixed tests

1531ec3

chriselion reviewed Jan 8, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

chriselion reviewed Jan 8, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

chriselion reviewed Jan 8, 2021

View reviewed changes

ml-agents/mlagents/trainers/trainer/rl_trainer.py Outdated Show resolved Hide resolved

chriselion reviewed Jan 8, 2021

View reviewed changes

ml-agents/mlagents/trainers/torch/components/reward_providers/gail_reward_provider.py Outdated Show resolved Hide resolved

chriselion reviewed Jan 8, 2021

View reviewed changes

Project/Assets/ML-Agents/Examples/Hallway/Scripts/HallwayAgent.cs Outdated Show resolved Hide resolved

brccabral added 3 commits January 7, 2021 18:45

Unity coding standard

dadf67d

reverted docstring empty lines

b5e6397

GaugeWriter fix

913fddf

Chris Elion added 4 commits January 8, 2021 09:40

revert some whitespace

8bc9892

undo whitespace

cc76bea

undo undesired change, undo whitespace

31f400d

revert unit test logging strings

eaf51a8

chriselion reviewed Jan 8, 2021

View reviewed changes

changelog

201f099

chriselion approved these changes Jan 8, 2021

View reviewed changes

chriselion merged commit f813287 into Unity-Technologies:master Jan 8, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SUM as aggregation type for custom statistics #4816

Added SUM as aggregation type for custom statistics #4816

brccabral commented Jan 3, 2021 •

edited

Loading

chriselion left a comment •

edited

Loading

chriselion Jan 6, 2021

brccabral Jan 6, 2021

chriselion Jan 6, 2021

chriselion Jan 6, 2021

brccabral Jan 6, 2021

chriselion Jan 6, 2021

brccabral Jan 6, 2021

chriselion Jan 8, 2021

chriselion Jan 8, 2021

chriselion Jan 6, 2021

brccabral Jan 6, 2021

chriselion Jan 6, 2021

brccabral commented Jan 6, 2021

chriselion commented Jan 6, 2021

chriselion Jan 6, 2021

brccabral commented Jan 7, 2021 •

edited

Loading

chriselion Jan 8, 2021

chriselion commented Jan 8, 2021

brccabral commented Jan 8, 2021

chriselion Jan 8, 2021

chriselion Jan 8, 2021

chriselion commented Jan 8, 2021

		log_info.append(f"Std of Reward: {stats_summary.std:0.3f}")
		log_info.append(f"Num of Reward: {stats_summary.num:0.3f}")

Added SUM as aggregation type for custom statistics #4816

Added SUM as aggregation type for custom statistics #4816

Conversation

brccabral commented Jan 3, 2021 • edited Loading

chriselion left a comment • edited Loading

Choose a reason for hiding this comment

Option 1

Option 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brccabral commented Jan 6, 2021

chriselion commented Jan 6, 2021

Choose a reason for hiding this comment

brccabral commented Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

chriselion commented Jan 8, 2021

brccabral commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriselion commented Jan 8, 2021

brccabral commented Jan 3, 2021 •

edited

Loading

chriselion left a comment •

edited

Loading

brccabral commented Jan 7, 2021 •

edited

Loading