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

Added useful metrics #168

Merged
merged 8 commits into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@mlomb
Copy link
Contributor

mlomb commented Nov 29, 2018

Useful metrics discussed in the Discord server that could really help to analyze bots.

@snaar snaar requested review from Janzert and lidavidm Nov 30, 2018

GameStatistics &stats) {
(void)map;

stats.player_statistics.at(owner_id.value).ships_spawned++;

This comment has been minimized.

@snaar

snaar Nov 30, 2018

Contributor

Mixing tabs and spaces here.

{"final_production", final_production},
FIELD_TO_JSON(total_production),
FIELD_TO_JSON(max_entity_distance),
FIELD_TO_JSON(number_dropoffs),
FIELD_TO_JSON(interaction_opportunities),
FIELD_TO_JSON(ships_captured),
FIELD_TO_JSON(ships_given),
FIELD_TO_JSON(ships_spawned),

This comment has been minimized.

@lidavidm

lidavidm Nov 30, 2018

Contributor

While we're at it, we should stop writing unused fields - ships_captured, ships_given, total_mined_from_captured, and perhaps interaction_opportunities.

This comment has been minimized.

@Janzert

Janzert Dec 1, 2018

Collaborator

I actually specifically advised against that on discord. But after thinking about it some more the risk seems very minimal and while the benefit is also fairly small it probably is good on balance.

Since you're obviously in favor, I'm fine with them going as well.

game.game_statistics.player_statistics[player_id.value].carried_at_end += game.store.get_entity(_entity_id).energy;
}
}

This comment has been minimized.

@Janzert

Janzert Dec 1, 2018

Collaborator

If a bot is terminated (crashes/times out) should the halite carried at that point be counted for carried_at_end?

I could see arguments both ways and I'm not sure which is more useful for a general stat. 'Last turn alive' is a strict superset of 'game end', so the latter could be derived from the former if desired. But it would entail extra processing later if what everyone really wants is the game end number and not the last turn bot was alive.

The current implementation only counts it at the game end and will always have zero for bots that are terminated early.

@@ -524,6 +532,8 @@ void HaliteImpl::update_player_stats() {
player_stats.turn_deposited.push_back(0);
}
}
// Used to track the current turn number inside Event::update_stats
game.game_statistics.turn_number = game.turn_number;

This comment has been minimized.

@Janzert

Janzert Dec 1, 2018

Collaborator

Turn numbering is confusing and maybe I'm getting it wrong; but it seems like this is introducing an off by one for the recorded turn numbers. Since the turn number is being set at the end of the turn here and then used throughout the next turn.

I think the correct place to do this is around line 143 when the logging turn number is also set. That way the turn number is set before the turn is processed.

@@ -245,9 +252,11 @@ int main(int argc, char *argv[]) {
results["terminated"][to_string(player_id)] = player.terminated;
}
}


results["execution_time"] = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now() - engine_start).count();

This comment has been minimized.

@Janzert

Janzert Dec 1, 2018

Collaborator

I would like to see this added to the game_statistics. That way it will be recorded in the replay file and server database.

The server will just ignore (and drop) it in the results from the engine. If it's useful in results for local testing I'm fine with leaving it there as well.

It would also be nice to get total time and maximum turn time used for each bot (we had one or both of those in h2). But that isn't trivial and doesn't need to be done in this PR.


if (map.at(location).owner != Player::None) { // There is a dropoff
player_stats.dropoff_collisions++;
}

This comment has been minimized.

@Janzert

Janzert Dec 1, 2018

Collaborator

Should dropoff_collisions really count collisions over all dropoffs? It seems like only counting collisions over the player's own dropoffs might be more useful.

@@ -499,10 +501,17 @@ void HaliteImpl::update_player_stats() {
if (!player.terminated && player.can_play) {
if (player_can_play(player)) { // Player may have died during this turn, in which case do not update final turn
player_stats.last_turn_alive = game.turn_number;

// Calculate carried_at_end

This comment has been minimized.

@lidavidm

lidavidm Dec 3, 2018

Contributor

(Watch out for mixed spaces/tabs here.)

@@ -149,6 +150,12 @@ int main(int argc, char *argv[]) {
hlt::Replay replay{game_statistics, map_parameters.num_players, map_parameters.seed, map};
Logging::log("Map seed is " + std::to_string(map_parameters.seed));

for (const auto &row : map.grid) {

This comment has been minimized.

@lidavidm

lidavidm Dec 3, 2018

Contributor

(Mixed tabs and spaces here too.)

@Janzert

Janzert approved these changes Dec 6, 2018

Copy link
Collaborator

Janzert left a comment

Looks good to me.

A couple minor things I noticed. Normally I'd say the commented out lines should just be deleted, but maybe we want to leave them in this case so someone running a competition with those mechanics can easily enable the relevant stats. Also there are at least a few otherwise blank lines that contain whitespace.

@lidavidm lidavidm merged commit e6c86e9 into HaliteChallenge:master Dec 6, 2018

@lidavidm

This comment has been minimized.

Copy link
Contributor

lidavidm commented Dec 6, 2018

Thanks @mlomb and @Janzert!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment