allfed / allfed-integrated-model

Integrated model to calculate the effects of resilient foods in catastrophic events
https://allfed.github.io/allfed-integrated-model/
GNU General Public License v3.0
11 stars 10 forks source link

`assert_fewer_people_fed_or_less_overall_food()`: Add seaweed, cs, or methane scp sometimes actually reduces people fed #159

Open morganrivers opened 1 week ago

morganrivers commented 1 week ago

see tests/test_argentina_parameters.py assert_fewer_people_fed_or_less_overall_food()

There's a minor bug in the second round optimization (to_animals): it seems to refuse to include SCP and
    seaweed and cellulosic sugar in the feed for animals in that round, yet the third round has no such problem.
    As a result, the second round, where feed is allocated, does not allocate sufficient feed. And when the third round
    is run, suddenly there's more feed and fewer people fed. So when we add seaweed, cs, or methane scp to the scenario
    sometimes this actually reduces people fed. It's also the case when we relocate crops and seaweed, cs, or scp are
    enabled.
morganrivers commented 1 week ago

Issue #140 is a subset of this bug

morganrivers commented 1 week ago

So to reproduce the issue, you can replace the test with a more stringent test: So replace the assert_fewer_people_fed_or_less_overall_food() with the following:

def assert_fewer_people_fed_or_less_overall_food(
    result_smaller,
    result_sum_total_smaller,
    result_bigger,
    result_sum_total_bigger,
    assert_message,
):
    """
    There is a problem: the INCREASE_FEED boolean in parameters sometimes guesses wrong about the amount of meat
    obtained, and over-adjusts feed. So in theory, this test may occasionally fail. But so far so good.
    If it does fail, try setting INCREASE_FEED to false.

    BUG: Also, there's a minor bug in the second round optimization (to_animals): it seems to refuse to include SCP and
    seaweed and cellulosic sugar in the feed for animals in that round, yet the third round has no such problem.
    As a result, the second round, where feed is allocated, does not allocate sufficient feed. And when the third round
    is run, suddenly there's more feed and fewer people fed. So when we add seaweed, cs, or methane scp to the scenario
    sometimes this actually reduces people fed. It's also the case when we relocate crops and seaweed, cs, or scp are
    enabled.
    """
    assert result_smaller <= result_bigger + test_tolerance, f"{assert_message}"

Of course, the test before was passing when it looked like this:


    if result_smaller <= 100 or result_bigger <= 100:
        # less than 100% fed! must be fewer people fed in the smaller scenario
        assert result_smaller <= result_bigger + test_tolerance, f"{assert_message}"
    else:
        # in case >= 100%, sometimes more food just goes to animal feed. And the result is smaller to humans,
        # but still no-one starves.
        assert (
            result_sum_total_smaller
            <= result_sum_total_bigger * test_tolerance_sum_total
        ), f"{assert_message}"

The fact it was passing when % fed was less than 100% tells us that, at least for all variations of scenario options tested, for the country of Argentina, when people are starving adding these resources never reduced the % fed by more than 1%. Furthermore, taking a look at how sum total and percent people fed are defined:

    percent_people_fed = interpreted_results_of_interest.percent_people_fed
    sum_total_kcals = (
        interpreted_results_of_interest.feed_and_biofuels_sum
        + interpreted_results_of_interest.to_humans_fed_sum
    )

we can see that the case where >100% are fed, there is still a test being run in such scenarios the production was not decreasing by more than 1% (feed + biofuel + to humans).

More test coverage might be nice though, as Argentina is unusually high production so most scenarios end up without people starving.

Running now to confirm the bug does indeed occur when the test also is run if % fed is >100%

morganrivers commented 1 week ago

The way to address this is probably to figure out why round 2 is not getting feed increased sufficiently, under the test conditions described above.

There are 3 specific failure points out of 216 "sanity check" tests.

assert_message: Including relocated crops cannot decrease the number of people fed
result_smaller: 177.37580895584804
result_bigger: 172.1383904436093

assert_message: Adding intake constraints cannot decrease the number of people fed
result_smaller: 192.23429619958534
result_bigger: 185.62247665628925

assert_message: Reducing crop production due to nuclear winter must result in fewer people fed
result_smaller: 177.37688894345726
result_bigger: 171.70548454478524

The relocated area was better in that there was no relocated area bug, but then the "Including relocated crops" error occurred instead