Refactor ResourcesAI.py #580
Conversation
Create separate functions for growth specials, research and industry specials, protection focus, happiness focus (empty placeholder) and industry and research foci. Thread variables through new functions to clarify which function changes which variable.
Move adj_round 1, calculate minimum possible R/I ratio into its own loop.
The code to exclude research focus for high industry planets was indented to the depth of the debugging header and only ran when the header was printed.
…e_with_underscore Renamed newTarget, currentFocus, currentOutput and planetMap to new_targets, current_focus, current_output and planet_map Removed treatment of newTargets as a pseudo global from AIstate
PlanetFocusManager replaces the file scope variables new_targets, current_focus, current_output and planet_map. It also replaces new_foci. focus_manager is an instance and is limited in scope to the set_planet_resource_foci function. focus_manager is threaded through the functions that previously accessed the file scope variables.
set_future_focus sets the focus if possible and removes the planet from the list of planets to be considered for further focus changes.
Created PlanetFocusInfo object to contain all information about a focus change and transfer these objects from raw (unset) to baked (set) state. This makes explicit the implicit assumption that planet_ids only referenced planets whose focus had not been decided. Added to PlanetFocusManager three tables all_planet_info, raw_planet_info and baked_planet_info. As the algorithm proceeds planet info is moved from the raw table to the baked table using bake_future_focus(pid, focus). Added the PlanetFocusInfo object to encapsulate all the information needed to decide how to set the focus. Removed the PlanetFocusManager members new_targets, current_focus, current_output, new_foci whose information is now contained in the individual planet infos. bake_future_focus(pid,focus) is the function that transfers planet info from the raw_ list to the baked_ list.
Moved most reporting into a Reporter class reporter.capture_section_info() captures which planets' focus was decided after each of the following steps: "Unfocusable", "Specials", "Protection", "Happiness" and "Typical" Sections of the table are only printed if they exist.
Ran pylint and fixed all 'Wrong continued indentation before block' errors to conform to PEP8
In ResourcesAI.Reporter compute the cumulative industry and research targets and min/max boundaries in the reporter instead of passing them from elsewhere.
| """Set the focus and moves it from the raw list to the baked list of planets | ||
| Return success or failure""" | ||
| pinfo = self.raw_planet_info.get(pid) | ||
| success = (pinfo != None and |
There was a problem hiding this comment.
https://www.python.org/dev/peps/pep-0008/#id45:
Comparisons to singletons like None should always be done with is or is not , never the equality operators.
PS. Just run code checker to fix all style issues at once.
|
Thanks for the review. What code checker are you using? I used pylint. I ignored variable naming violations, missing docstrings, too many branches and too many statements as there were already violations in the old code. It did not pick up the violations that you pointed out, use of None etc. The last string formatting code was in the legacy code and was only moved. I didn't write it and wish to dodge the blame. I did fix it as requested. |
|
I use embedded checker in PyCharm (Community edition is free). It highlight warnings in code. | The PR fixes one small bug preventing more than one high industry planet from being prevented from switching to research and otherwise doesn't intentionally change any logic. Can you rephrase phrase in description because I don sure that I understand it properly You can use See also Styling with Markdown is supported link at the bottom of edit area, there is a lot of useful text formatting features. You can use real list instead of manually creating them. And make sublists inside them. |
| itarget = pinfo.planet.currentMeterValue(fo.meterType.targetIndustry) | ||
| rtarget = pinfo.planet.currentMeterValue(fo.meterType.targetResearch) | ||
| pinfo.possible_output[focus] = (itarget, rtarget) | ||
| return success |
There was a problem hiding this comment.
PyCharm generates me next hint:
Inferred type: (self: PlanetFocusManager, pid: Any, focus: Any, update: bool) -> Union[bool, int]
Just wrap success to bool before return it.
|
Cjkjvfnby
added
category:refactoring
| universe.updateMeterEstimates(self.raw_planet_info.keys()) | ||
| itarget = pinfo.planet.currentMeterValue(fo.meterType.targetIndustry) | ||
| rtarget = pinfo.planet.currentMeterValue(fo.meterType.targetResearch) | ||
| pinfo.possible_output[focus] = (itarget, rtarget) |
There was a problem hiding this comment.
I'm unsure about the logic of the entire block here.
First, you remove pid out of raw_planet_info, then you update meter estimates but only for the raw_planet_info, thus not pid, yet try to update pinfo which is paired with pid?
There was a problem hiding this comment.
The pinfo object is transferred from the raw_planet_info to baked_planet_info. That same pinfo object is the one that is updated.
There was a problem hiding this comment.
Yes... But pid is not included in the updateMeterEstimates call, is that intentional?
There was a problem hiding this comment.
You are correct.
It was intentional, but then I moved the code to grab the meterValues for the current planet to after the point where pinfo was transferred.
There was a problem hiding this comment.
calculate_planet_infos already calculates all planet info so we can remove pinfo.possible_output[focus] = (itarget, rtarget) form here
There was a problem hiding this comment.
calculate_planet_infos() is called after all of the planet foci with global effects are set.
However, bake_future_focus() is called to set the planets with global effects (before calculate_planet_infos is called) and at that point they still need their pinfo.possible_output[focus] to be calculated.
The method avoids having to re-calculate all of the planets possible_outputs after every focus setting event.
1. Add note about lack of stability wrt pid ordering to replace commented shuffle code. 2. Improved docstring for bake_future_focus, to clarify usage of update. 3. Fixed bake_future_focus and made it remove current planet from raw planets after updating meters. 4. Made all docstrings use """ not '''. 5. More PEP8
LGM-Doyle
force-pushed
the
refactor_ResourcesAI_py
branch
from
ad150a2
to
c8cd5d2
Compare
| self.current_output[IFocus] = planet.currentMeterValue(fo.meterType.industry) | ||
| self.current_output[RFocus] = planet.currentMeterValue(fo.meterType.research) | ||
| self.current_output = (planet.currentMeterValue(fo.meterType.industry) | ||
| , planet.currentMeterValue(fo.meterType.research)) |
There was a problem hiding this comment.
Leave coma at previous line
1. Add space after # in comments 2. Move split line commas to end of previous line. 3. Clarify comment about target vs current population in enabling focus
|
Hi, LGM-Doyle, the refactoring looks good to me. Also, nice job catching the bug, indentation errors can be tricky enough that even after your initial note above I still had to look pretty close to spot the bug and fix. That might actually wind up being a pretty notable bug fix, I'm interested in seeing how that works out. Also, in that section of code the entire use of the variable "do_research" is just a holdover from a previous way of tracking preset focii; in the current code (and your refactoring) all uses of do_research could simply be removed. If you had responded to Cj's question about the bug description, I'm not seeing it. I would phrase it as "The PR fixes an indentation error which prevented more than one high production planet from bypassing Research Focus during the final assessment of the local research_potential : industry_potential ratio." To confirm that the refactoring itself did not cause any unintended changes of focus assignments, did you do any save-game testing without the bugfix in place? |
| #TODO Could this be changed to current population instead of target population? | ||
| #It uses the target population to avoid situations where an unsustainable population is dying? | ||
| # It uses the target population versus current population to avoid situations | ||
| # where an unsustainable population is dying? |
There was a problem hiding this comment.
It's my recollection that a while in the past, when a colony died down to an outpost it still retained its last-set focus, which could not be changed, hence a test like the one here (though I don't recall why it would look at target pop rather than current pop). Nowadays the focus is reset to nothing in that case, so I think this test would no longer be needed (or perhaps simply be changed to look for nonempty AvailableFoci).
…Foci planet.availableFoci is now empty when a planet can't focus.
|
Dilvish, thanks for the review. I have removed do_research and can_change_focus(). I answered CJ's question above. It is one of the collapsed comments on my browser. Do you want me to change the commit message? I thought the change in the code was clear. To check that I didn't break anything as I refactored I looked for automated tests. There only appears to be a testing build target in the CMakefiles for the parser. So, here is how I tested. Before I changed anything I ran 150 turns of a random but specific universe. I picked two empires and observed their progress: one that grew successfully (AI4) and one that did not (AI3). I kept their AI_N.log files as an example of a correctly working resource allocation. I ran the same universe with ResourcesAI completely disabled and took note of the results of complete failure. Then I added noisy debugging print statements, since removed, to adj_round 2, 3 and 4 and the ratio loop just past their guard conditions. That is how I noticed that the header printing was masking further research assignments in the ratio portion of the algorithm. After the refactor before I removed the noisy debugging statements AI3 and AI4 were still developed along similar lines and were toggling foci in a similar manner. As the refactor was extensive, and there is no automated testing I can NOT say that I haven't introduced any new bugs. |
|
I still have question:
Last time I try to automate test, I was stopped by some randomness that can-not be controlled by AI. I don't remember what exactly it was. |
|
Cj, here are some of my quick thoughts/questions on testing the AI. For testing the AI, I have only looked at ResourcesAI.py. It is definitely influenced by the globals, foAI, empire and universe. One way to make testing repeatable would be to have fake/mock foAI, empire and universe objects that just feed in expected test data and respond non-randomly to expected changes or fail the test. I would be interested in adding testing as a developer tool to the python: adding a test_AI target to the CMake files etc. I would not be interested in trying to write tests for all of the legacy code. What is your preferred python testing framework? Should I create an AI testing github issue even if it is an umbrella issue that is never checked off? |
|
@LGM-Doyle, you mean unittest for code, executed outside of the AI? This require to split AI to modules (which is nice). For python I prefer pytest. It full of magic but do a lot of work for you. On windows machine I does not manage to make it work inside AI. (freeorion use own python for game proposes for win builds) According to branch I have no comments left. Lets merge it. |
|
@Dilvish-fo, @Morlic-fo, any objections about merge it? |
| """Assess and set planet focus to preferred focus depending on happiness""" | ||
| # TODO Assess need to set planet to preferred focus to improve happiness | ||
|
|
||
|
|
There was a problem hiding this comment.
function seems missing a pass statement.
There was a problem hiding this comment.
pass required in no other code is present. It just like close bracket for indent.
There was a problem hiding this comment.
Do comments actually count as well? Cause there sure isn't any actual code within the function.
There was a problem hiding this comment.
Comments does not count. Function have docstring. It is part of function declaration and can be accessed in runtime via __doc__
1. Clarified function purpose by changing function names use_planet_growth_specials to set_planet_growth_specials and use_planet_production_and_research_specials to set_planet_production_and_research_specials. 2. Corrected naming of spSize to _spPop to indicate it is unused and refers to population size, not planet size. 3. Expanded docstring of use_planet_production_and_research_specials to clarify its use. 4. Removed dead duplicate error reporting code for concentration camp focus setting. 5. Added pass to set_planet_happiness_foci placeholder function.
|
@Morlic-fo thanks for the review. I changed everything except the variable names, which I left the same as before the refactor. |
|
I have no further objections to merging this @Cjkjvfnby |
This PR is a major refactor of ResourcesAI.py.
I was trying to determine why the AI appears to be precessing around a threshold when toggling between research and industry focus every third turn. It turns out adj_round 3 intentionally trades 3 industry points for 1 research point when targeting low a research/industry ratio.
The PR fixes one small bug preventing more than one high industry planet from being prevented from switching to research and otherwise doesn't intentionally change any logic.
The refactor is mostly of three parts.
I think the refactored code is clearer to read.
The code is more local in scope and hence easier to modify in future.