Bug - Confirmed Maximizer didn't equip a weapon

[fronobulax]

Found and fixed the password hash.

https://github.com/kolmafia/kolmafia/pull/2203 has a single passing test added that to the best of my ability demonstrates the Maximizer selecting and equipping a weapon. I am hoping for feedback. If that test is the best we can do in a test that doesn't actually execute requests then I need to instrument the code and run it live. Otherwise my quest for a failing test can continue with some hope of success

 

[MCroft]

It looks like the test is calling the 2 parameter version of maximize. It looks like Autoscend is calling the 4 param version .

void equipMaximizedGear()
{
    finalizeMaximize();
    maximize(get_property("auto_maximize_current"), 2500, 0, false);
    // below code is to help diagnose, debug and workaround the intermittent issue where the maximizer fails to equip anything in hand slots
    // if this is confirmed as fixed by mafia devs, remove the below code.
    if (equipped_item($slot[weapon]) == $item[none] && my_path() != $path[Way of the Surprising Fist]) {
        // do we actually have a weapon we can equip?
        item equippableWeapon = $item[none];
        foreach it in get_inventory() {
            if (it.to_slot() == $slot[weapon] && can_equip(it)) {
                // found a weapon and we should be able to equip it.
                equippableWeapon = it;
                break;
            }
        }
        if (equippableWeapon != $item[none]) {
            auto_log_error("It looks like the maximizer didn't equip any weapons for you. Lets dump some debugging info to help the KolMafia devs look into this.");
            addToMaximize("2 dump"); // maximizer will dump a bunch of stuff to the session log with this
            maximize(get_property("auto_maximize_current"), 2500, 0, false);
            removeFromMaximize("2 dump");
            if (equipped_item($slot[weapon]) == $item[none]) {
                // workaround. equip a weapon & re-running maximizer appears to fix the issue.
                equip(equippableWeapon);
                maximize(get_property("auto_maximize_current"), 2500, 0, false);
                auto_log_error("No weapon was equipped by the maximizer. If you want to report this to the mafia devs at kolmafia.us include your session log. We have attempted a work around.");
            }
        }
    }

 

[fronobulax]

The ash 2 parameter version calls an internal 5 parameter version with zeros for the price data meaning the max price is zero and the maximizer should not consider buying.

The ash 4 parameter version calls an internal 5 parameter version that interprets the third parameter as zero as meaning the maximizer should not consider buying.

Regardless of how it got there the internal five parameter version calls an internal four parameter version which does all of the work.

So both versions get to the same code.

Since zero for price level makes the maximum price irrelevant

maximize(get_property("auto_maximize_current"), 2500, 0, false);

is the same as

maximize(get_property("auto_maximize_current"), 0 , 0, false);

which is the same as the two parameter call

maximize(get_property("auto_maximize_current"), false);

All that said RunTimeLibraryTest in the draft PR now has itShouldMaximizeAndEquipSelectedWeapon4Parameter which is a copy of itShouldMaximizeAndEquipSelectedWeapon that calls the 4 parameter version instead of the two parameter version.

I may have discovered a case where two tests that I think should be identical are not. I will be figuring that out

 

[fronobulax]

Well. There is a helper, getBoosts that reports what the maximizer recommends. It turns out it references a static member of maximizer and one of my tests was incorrectly constructed as a result. Curious.

 

[fronobulax]

Current thinking and tests and a PR may change.

There are two obvious causes of the maximizer being run and failing to equip a weapon. One is that the maximizer did not select a weapon. I originally thought the "effective" keyword was involved but I no longer think so. I am pretty sure I do not have data or a test where no weapon is selected and that is the "wrong" answer but in light of things I now believe, I will revisit that.

The other case is the selected weapon is not equipped. I thought I had a lot of instances where this might be happening but it turns out that my tests do not actually allow/force the Equip command to execute to the point that testing whether the item is equipped passes. Checking Boosts will show what was selected but the current implementation is that if the equip command is emitted then the item is no longer listed in Boosts.

So I am going to revisit/clean up tests so that checking Boosts is a surrogate for selection but not actually equipping, investigate failures to select that still exist and see if I can get a repeatable failure to equip test. If not then I guess I need to run live in the debugger or find a test that simulates more aspects of Equip.

 

[fronobulax]

Cleaned up the PR. One failing, disabled, test that I want to understand. Everything else works as expected. The new tests are probably redundant but someone can ding me on that once it goes back for review

Boosts are not the thing to look at if you are speculating. Once I realized that some of the tests became irrelevant and were deleted. I did keep the change to getBoosts to return a copy of Boosts because there was leakage when I didn't even though none of the current tests fail because of that.

 

[fronobulax]

That was awkward. I had a breakpoint where I thought a weapon had been selected. I had another where the equip command was emitted. The first fired as expected. The second didn't. So now I have to figure out the execution path and decide whether there is unnoticed or unexpected code keeping the second from executing or whether it is just operator error.

 

[fronobulax]

Debugger was not providing any insight.

The "2, dump" from autoscend was not as helpful as I would like because I wasn't mirroring and the output did not go to the session log. But just running and dumping I notice "2, dump" is offering to put things in places where they don't belong - The Nuge's favorite crossbow as SLOT ACCESSORY3, for example. It could be that I am entirely misunderstanding how to read the dump output. But it could also be that the mapping between equipment and SLOTS is getting hosed.

 

[fronobulax]

Well the weird slot assignment is deliberate. It is part of a scheme to keep the number of slots manageable.

I keep finding examples where no weapon and weapon score the same and so no weapon is selected.

 

[ziz]

Well the weird slot assignment is deliberate. It is part of a scheme to keep the number of slots manageable.

Someone mentioned recently that this was long-ago decided for early-Mafia memory-efficiency reasons.

Is that still a relevant concern now, fifteen years on? (ref: the commit that says it introduced Modifier Maximizer was made on Jul 13, 2009) That is, obviously memory efficiency is always a concern; is this specific efficiency choice still serving a more valuable purpose than simpler/clearer/easier to debug code would be at this point?

 

[fronobulax]

Someone mentioned recently that this was long-ago decided for early-Mafia memory-efficiency reasons.

Is that still a relevant concern now, fifteen years on? (ref: the commit that says it introduced Modifier Maximizer was made on Jul 13, 2009) That is, obviously memory efficiency is always a concern; is this specific efficiency choice still serving a more valuable purpose than simpler/clearer/easier to debug code would be at this point?

As a philosophy discussion that is a good question and it is one of several areas where it things were newly done today there would be different decisions. On a practical basis the Maximizer is not something that is easily understood or changed. If I had the resources I would consider reformulating the entire process as a (mathematical) optimization process solvable by any one of several algorithms. But there is no guarantee that all of the existing use cases could be captured in such a formulation.

If I can identify a problem caused by reusing slots then I will certainly consider not reusing them as a possible solution. But that may end up reducing code complexity and in creasing runtime so it might not be practical. I have only scratched the surface but the only bad thing I have observed is that the output from "2 dump" is confusing and hard to interpret.

Right now every case I have found where a weapon is not equipped is caused by the maximizer not choosing a weapon. In most of those cases the maximizer is correct - using the "best" weapon does not produce a better score than not using it. I need to review some of the cases because it is possible that I did not capture them as expected. I am also thinking I need to put in some logging statements since the debugger can get tedious.

 

[fronobulax]

Everything you think you know is probably wrong.

I went back to the data in the OP and decided to make a test that replicated it. I thought I had done something similar but when you run out of ideas...

I also unconditionally forced "2 dump" for the WEAPON slot only, whenever the code was executed.

So my test chooses to equip a disco ball and correctly emits the commands to do so. But my print only shows NONE and toy accordion being considered as weapons (and suggesting the accordion).

A difference between the command line and the GUI is that the command line will prune the list of recommendations as they are equipped.

So am I looking at the wrong bits of code? Is "2 dump" not the definitive source for what was recommended? In spite of my efforts do my tests exercise a different code path than running live?

Not done yet but...

 

[fronobulax]

Still here. I do not have a repeatable case - manually or via tests - where the Maximizer recommends a weapon and then it is not equipped. I have several cases where a weapon is not recommended and I believe that recommendation is correct. I have questions about the sequencing of things in code. For example sometimes it seem like "2 dump" doesn't tell me everything I expect and I don't know whether that is my understanding or a code glitch. But I'm not making progress and what I really need to do is block out several hours and decide what I know, what I don't know and what to do about it.

 

[fronobulax]

Welcome to the snail races. As far as I can tell the cases where the maximizer does not equip a weapon are cases where the weapon contributes nothing to whatever is being scored. So now I have to figure out why the weapon does not contribute and then decide whether that is correct or not. For example I wonder if something is prematurely excluding weapons from consideration. No answers yet but not giving up.

 

[fronobulax]

Accountability.

I still do not have a reproducible case where the maximizer recommends a weapon, is allowed to equip and fails to equip the recommended weapon. Still have some things to try and help find such a case, but...

 

[fronobulax]

Still no viable test case where a weapon is selected but not equipped. I still poke at things once in a while but since I'm not running autoscend a lot I don't get reminded that there is still a concern as often as I perhaps should. My project to make a MiniMaximzer that only considered on hand equipment, basic keywords and iterated through all combinations has stalled because scoring a set of equipment is harder than I first thought. But I'd still like to see it work since some of the issues I have to think about are helping me think about the Real Maximizer and how it should behave.