Merge lp:~widelands-dev/widelands/ai_productionsites_statistics into lp:widelands

[TiborB]

Reworking the way how AI internally calculates utilization of productionsites.

This is change in productionsites object, but the output is used only by AI. It tries to reflect the duration of production programs, or time of skipping or failing... We can discuss this, but should be more accurate than what is in code now...

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5020. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/534575650.
Appveyor build 4801. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ai_productionsites_statistics-4801.

[GunChleoc (gunchleoc)]

As I wrote on the forums, I think that this should be calculated when parsing the production programs. This way, we get more accurate statistics, and we can use it in the encyclopedia.

This has been in my head for a while and I plan to start working on this once the 2 prerequisite branches are in, so I'd rather not add new code that we'll then need to remove anyway.

[hessenfarmer (stephan-lutz)]

see inline comments

[TiborB (tiborb95)]

What I need here is % of time utilization, something more accurate that what is now calculated. I know that times are deducted from LUA files, but it seems easier to add one variable here. Also some production programs can vary in duration, correct?

[TiborB (tiborb95)]

@hessenfarmer - see my responses

[GunChleoc (gunchleoc)]

I guess the question here is - what are you using this for? Do you need the exact duration of a specific run of the production program, or do you need to know how long it usually takes?

[TiborB (tiborb95)]

AI need % of building utilization. And I think showed % (in UI) is not reliable enough...
By old approach it just presumed that successful program took 5x more than unsuccessful program - this is example. With exact timing the number would be more exact.
Also we have 'skipped' and 'failed' programs that takes various time I think.
This change guarantee that whatever changes to productions programs this code will not need an update...

[hessenfarmer (stephan-lutz)]

Just had a second look. Now we do count a skipped program as a fail which hasn't be the case in the previous version.
From what I can see in the old version the algorithm was:

if failed --> scale the old stats to 80% (resulting in asymptotically falling values if fail everytime)

if skipped --> scale old percentage to 98% (which is basically stable for a lot of time)

if success --> scale to 80 % of previous and add 20 % (resulting in asymptotically rising values)

the UI percent is not really a percentage of successful time but a percentage of successful attempts. Which mean it does not reflect different length of programs. The UI also provides a trending. So for me it might be worth a try to use the UI percentage, cause it only updates if needed (failed or complete) and we removed the effect of skipping on stats with one of the balancing branches thanks to stonerl inventing the state no stats. So for me the UI stats are pretty reliable now in terms of successful attempts.

[TiborB (tiborb95)]

But if failure (whatever) takes 5 seconds and success 90, then after 2 failures and 1 success, UI statistics will be 35%, but the productionsite was actually working 90% of time....

[hessenfarmer (stephan-lutz)]

Correct. I think the question is what is the relevant Information. I believe the information necessary for management is how good is a building satisfying its intended task. For me in this case it is more relevant the relation between success and failure rather than the relation active to not active. in your example with 2 successes and 1 fail the activity would be 97% while still failing every third attempt to produce something. This might even be worse if it has more than one outputs . It might be that it fails completely to produce one ware_type while having stats of 97%.

[TiborB (tiborb95)]

The question to answer here is - do we need another building of the type. If time utilization is 35 % - we dont need.
If 97% - we definitely need.

[hessenfarmer (stephan-lutz)]

do we need another building also is task based from my perspective not time based.

if we fail every 3rd attempt we don't need another building as we can't provide enough inputs for the current one. Fails are rather short now for a lot of buildings, as we have updated the working programs of them to fail immediately if not enough wares are present. means we are not sleeping useless time to just fail, but continue with other programs, or recheck if we can do it now.
So I really would vote for a task based utilization scheme rather than having a time based.

[TiborB (tiborb95)]

But we are still talking only about info for AI not players....

If we cannot produce something due to missing inputs and suddenly we will have those inputs, the productionsite will have to reduce production of the ware that is being produced 90% of time now. Might not be bad, but we will be suddenly on 100 % and new building might be needed definitely....
If I remember properly skips and failures took about 5 seconds so are not that short...

[hessenfarmer (stephan-lutz)]

skips take 10 seconds if second time skipped this is hardcoded in productionsite.cc

fails do not need any time (except some milliseconds) for e.g. atlantean toolsmithy

If we use UI stats we will not go to 100% suddenly cause we take the last 20 task into account. so if we have currently failed every third attempt and now we manage constantly to be successful we will slightly raise productivity to 100% until we have finished 20 tasks without failing one.

As aid there was a fix for the stats of productionsites last year. now we have somewhat reliable task based statistics in the UI I believe.

So my suggestion is to use the Stats for the players for the AI as well. I think the resolution of 5% is good enough to base Ai decisions on.

[TiborB (tiborb95)]

I dont say that official statistics is completely wrong. But rather it looks at the thing from other angle...

Also you said it will rise up slowly, but will fall down much faster, correct?

[TiborB (tiborb95)]

I see the opinions differ too much. I see these options:

- no change
- this change
- no change, but expose official statistics to the AI's genetic algorithm and retrain
- this change, but expose official statistics to the AI's genetic algorithm and retrain
- remove this crude_statistics, and expose official statistics to AI and retrain

[hessenfarmer (stephan-lutz)]

UI stats are calcualted over the last 20 real attempts. Skipped does not count as attempt. It is build like a shifting register with 20 positions containing true or false. it is falling faster for most buildings due to the fail time is shorter than working time. if fail time is really short it falls down very fast in result (atlantean toolsmithy e.g) interesting part is we calculate 2 values: the percentage (number of true / 20) and the trend (number of true in last 10 attempts / 10).

From my perspective the only way to control a buildings productivity is the supply of wares. In fact we have the time of one work cycle to supply the wares for the next cycle. To cope with economy shortfalls we have a buffer of normally around 1 cycle in the input queues. So for me the attempt based statistics is a good indication whether we supply a building properly. An additional building is only reasonable if the current buildings are supplied properly.

[TiborB (tiborb95)]

I completely understand current code. If it was "attempts in last x minutes" - it would be fine for me. But 'last 20 attempts' - this I understand from coding perspective - but dont like it.

'official statistics' to me is like actual_statistics^2. 100% and 0% are right, but in between it is undervalued...

But I am willing to accept removal of this statistics and modify AI code and retrain it.... if there is consensus about it..

[hessenfarmer (stephan-lutz)]

Hm I made a excel now. reported a bug to upload it and linked it to this branch. It seems your formula is delivering weird results. I don't know why or if I made a mistake. I think I don't have understood this in total but at least the old crude_statistics wasn't that far from UI but more volatile.

Please have a look: if you don't like excel I can try to export in odt as well.

We could try to have a attempts in last minutes solution though. That would be probably the best

[hessenfarmer (stephan-lutz)]

Got the concept. I think we should have this after you perhaps might fix the small nits from the below diff comments

review: Approve

[hessenfarmer (stephan-lutz)]

by the way I now see another option.

- this change plus exposing the new crude Statistics to the UI ;-)

[TiborB (tiborb95)]

OK, comments implemented.

[hessenfarmer (stephan-lutz)]

Ok thanks unfortunately I found another thin to discuss.

How do we handle skipped programs? Currently we handle them as if they have failed. But we shouldn't handle them at all. Neither positive nor negative.

[TiborB (tiborb95)]

Low utilization can mean one of two shortage of inputs or that the output is not needed. So by my opinion, skipped programs should be included in statistics...

Because utilization close to 100 while the building being idle most of time can confuse players in believing that new building is needed...

[hessenfarmer (stephan-lutz)]

ok you are right especially for the AI cause the AI might want to build another idle building then.
However we should perhaps have different rating for failed and skipped. If you like we could deliver only half of the duration time to the update fuctionality in case we skipped so it will fall more slowly.
If you don't agree feel free to merge. I believe you tested this as I didn't

[TiborB (tiborb95)]

For AI this is the best I believe...

[hessenfarmer (stephan-lutz)]

Have done some playtesting 4 AI players on full moon.

found no obvious anomalies.

@bunnybot merge

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 5025. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/535462295.

[hessenfarmer (stephan-lutz)]

transient failure on travis

@bunnybot merge force

[GunChleoc (gunchleoc)]

I think skipped should be ignored in the UI, but the AI should be able to know whether a program has been skipped a lot recently, because that means that a new building is not needed.

=== modified file 'src/logic/map_objects/tribes/productionsite.cc'
--- src/logic/map_objects/tribes/productionsite.cc	2019-05-18 11:58:43 +0000
+++ src/logic/map_objects/tribes/productionsite.cc	2019-05-21 19:08:29 +0000
@@ -269,6 +269,7 @@
      statistics_(STATISTICS_VECTOR_LENGTH, false),
      last_stat_percent_(0),
      crude_percent_(0),
+     last_program_end_time(0),
      is_stopped_(false),
      default_anim_("idle"),
      main_worker_(-1) {
@@ -954,24 +955,28 @@
 		top_state().phase = result;
 	}
 
+	const uint32_t current_duration = game.get_gametime() - last_program_end_time;
+	assert(game.get_gametime() >= last_program_end_time);
+	last_program_end_time = game.get_gametime();
+
 	switch (result) {
 	case ProgramResult::kFailed:
 		statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
 		statistics_.push_back(false);
 		calc_statistics();
-		crude_percent_ = crude_percent_ * 8 / 10;
+		update_crude_statistics(current_duration, false);
 		break;
 	case ProgramResult::kCompleted:
 		skipped_programs_.erase(program_name);
 		statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
 		statistics_.push_back(true);
 		train_workers(game);
-		crude_percent_ = crude_percent_ * 8 / 10 + 1000000 * 2 / 10;
+		update_crude_statistics(current_duration, true);
 		calc_statistics();
 		break;
 	case ProgramResult::kSkipped:
 		skipped_programs_[program_name] = game.get_gametime();
-		crude_percent_ = crude_percent_ * 98 / 100;
+		update_crude_statistics(current_duration, false);
 		break;
 	case ProgramResult::kNone:
 		skipped_programs_.erase(program_name);
@@ -1032,4 +1037,17 @@
 
 	default_anim_ = anim;
 }
+
+void ProductionSite::update_crude_statistics(uint32_t duration, const bool produced) {
+	static const uint32_t duration_cap = 180 * 1000; //This is highest allowed program duration
+	// just for case something went very wrong...
+	static const uint32_t entire_duration = 10 * 60 *1000;
+	if (duration > duration_cap) {
+		duration = duration_cap;
+	};
+	const uint32_t past_duration = entire_duration - duration;
+	crude_percent_ = (crude_percent_ * past_duration + produced * duration * 10000) / entire_duration;
+	assert(crude_percent_ <= 10000); //be sure we do not go above 100 %
+	}
+
 }  // namespace Widelands
 
=== modified file 'src/logic/map_objects/tribes/productionsite.h'
--- src/logic/map_objects/tribes/productionsite.h	2019-05-11 12:37:45 +0000
+++ src/logic/map_objects/tribes/productionsite.h	2019-05-21 19:08:29 +0000
@@ -190,8 +190,13 @@
 	uint8_t get_statistics_percent() {
 		return last_stat_percent_;
 	}
+
+	// receives the duration of the last period and the result (true if something was produced)
+	// and sets crude_percent_ to new value
+	void update_crude_statistics(uint32_t, bool);
+
 	uint8_t get_crude_statistics() {
-		return (crude_percent_ + 5000) / 10000;
+		return crude_percent_  / 100;
 	}
 
 	const std::string& production_result() const {
@@ -322,8 +327,9 @@
 	std::vector<bool> statistics_;
 	uint8_t last_stat_percent_;
 	// integer 0-10000000, to be divided by 10000 to get a percent, to avoid float (target range:
-	// 0-10)
-	uint32_t crude_percent_;
+	// 0-100)
+	uint32_t crude_percent_; // basically this is percent * 100 to avoid floats
+	uint32_t last_program_end_time;
 	bool is_stopped_;
 	std::string default_anim_;  // normally "idle", "empty", if empty mine.