Skip to content
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

Remove randomness from metabolism #46906

Merged
merged 4 commits into from
Jan 24, 2021

Conversation

anothersimulacrum
Copy link
Member

@anothersimulacrum anothersimulacrum commented Jan 20, 2021

Summary

SUMMARY: Infrastructure "Remove randomness from metabolism"

Purpose of change

Make tests making assertions about calorie related things more reliable.

Describe the solution

Increase definition of calories to the calorie level, instead of the kcal or Calorie level.
Remove RNG from metabolism.

See commits for more detail.

Describe alternatives you've considered

Only increasing definition for the character.

Testing

See tests.

Additional context

This should really help with or close issues #46384, #46256 and probably #46473 (FYI @actual-nh).

@anothersimulacrum anothersimulacrum added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Items: Food / Vitamins Comestibles and drinks Mechanics: Character / Player Character / Player mechanics labels Jan 20, 2021
@actual-nh
Copy link
Contributor

actual-nh commented Jan 21, 2021

Very interesting! I am a bit too tired/zonked (labs with COVID restrictions are rather a headache...) right now to handle testing via merging with #46473's code. I might manage tonight simply downloading it locally to try --order rand subset testing here (my computer is too old and slow to frequently run all the tests).

@actual-nh
Copy link
Contributor

Currently compiling local version with random test ordering.

Copy link
Contributor

@actual-nh actual-nh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will recheck locally with these (once in) and also adding in restoration of calories to the healthy level between tests. (Will also take a look at Travis error(s).)

tests/weary_test.cpp Outdated Show resolved Hide resolved
tests/weary_test.cpp Outdated Show resolved Hide resolved
tests/weary_test.cpp Outdated Show resolved Hide resolved
tests/weary_test.cpp Outdated Show resolved Hide resolved
tests/weary_test.cpp Outdated Show resolved Hide resolved
tests/weary_test.cpp Outdated Show resolved Hide resolved
@actual-nh
Copy link
Contributor

actual-nh commented Jan 21, 2021

Travis (unexpected) errors were with the weary tests. Not sure why still getting machine-to-machine/OS-to-OS variations (ming was the one with errors); they should all be rounding the division by 1000 the same way, for instance... The ones that failed (and the --success ones I did locally) are still showing the fluctuation by weariness level at the start (probably due to weary.intake, from earlier findings in #46473).

I do think there are, nonetheless, improvements - I would normally expect a lot more test failures with randomized test order, given no setting stored calories to healthy calories and setting lots of margins to 0. (For that matter, I'd expect failures on a lot more builds with setting margins to 0, even without randomized test order, so that looks to be improved!)

@anothersimulacrum
Copy link
Member Author

Okay, good!
I'll clean this up and get it ready for merging then.
I might refactor the weariness tests to return transitions in turns, so that we can see just how much fluctuation there still is.

@actual-nh
Copy link
Contributor

BTW, do you happen to know why caloric intake is being factored into weariness?

@anothersimulacrum
Copy link
Member Author

The practical effect is that it allows working harder longer if you're keeping well fed. I'd need to go back to the issue and conversations with erk for more detail.

@actual-nh
Copy link
Contributor

Okay, good!
I'll clean this up and get it ready for merging then.

I would put the margins back to 5, BTW, from running with randomized order (but with stabilizing calories) - quite a few of the ones that are still erroring are off by 5.

I might refactor the weariness tests to return transitions in turns, so that we can see just how much fluctuation there still is.

I suspect that may require #45316 to be in, at the minimum. In #46473, I'm seeing the more-detailed variation via adding in getting (for each transition) both weary.tracker and weary.intake.

@actual-nh
Copy link
Contributor

The practical effect is that it allows working harder longer if you're keeping well fed. I'd need to go back to the issue and conversations with erk for more detail.

Understand. (I trust @I-am-Erk is OK, just understandably busy?) There seems to be a problem in that it is not simply overall "keeping well fed", but keeping well fed such that your guts are always contributing caloric intake. Someone without diabetes or hypoglycemia should have more-stable blood glucose (and other nutrient) levels than that. Keeping track of food intake over the past 24-48 hours would be the ideal fix for this, but a more practical solution would probably involve using an exponentially-decaying moving average, with alpha adjusted to be roughly equivalent to a 24 or 48 hour moving average.

@anothersimulacrum
Copy link
Member Author

anothersimulacrum commented Jan 22, 2021

If by food intake you mean calorie intake, it'd be super simple to keep track of that over the past 24-48 hours, it just requires a fair bit of testing/tweaking to make it work with existing functions (probably).

We don't need to crash here, we can just abort the test with REQUIRE
@anothersimulacrum
Copy link
Member Author

Cleaned up and ready for merge.

@lgtm-com
Copy link

lgtm-com bot commented Jan 22, 2021

This pull request introduces 2 alerts when merging f3c5587 into 43dfb22 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same

calorie precision is only used internally, all external methods should
be dealing with kcal.

Having calorie precision just enables removing rng-based rounding from
metabolism functions, without losing detail.

There was a very minor change in the Calories of some crafted items, as
is caught in the tests.
Having more precision enables removing rng from metabolism, without
losing detail (the rng was introduced through roll_remainder).

Calorie precision is only available privately - nothing outside of the
metabolism needs that level of precision.
With more precision now, we can drop rng based rounding!

Adjust the two places where this occurs, then adjust all tests to fit.
The weariness tests actually pass with margins of 0, but margin of 5 is
more safe for extenuating circumstances (e.g. unordered tests).
@actual-nh
Copy link
Contributor

If by food intake you mean calorie intake, it'd be super simple to keep track of that over the past 24-48 hours, it just requires a fair bit of testing/tweaking to make it work with existing functions (probably).

What mechanism do you have in mind for keeping track over a 24-48 hour period?

@anothersimulacrum
Copy link
Member Author

std::vector<std::pair<time_point>, int>
Every five minutes when food is digested, add an entry of { calendar::turn, intake }.
Then, either have the vector sorted, or just walk through it an delete everything that's too old.
Just sum the entire vector for intake.

@jbytheway
Copy link
Contributor

It would be nice to see calories promoted to a proper unit like mass, length, etc. (I guess we already have an energy unit, which would technically work, but probably clearer to have a separate calorie one).

@anothersimulacrum
Copy link
Member Author

We probably should have one. The pedant in me wants to use units::energy, but that is not particularly clear, yes. It does have appropriate precision, however.

wapcaplet added a commit to wapcaplet/Cataclysm-DDA that referenced this pull request Feb 13, 2021
Before CleverRaven#46906, character templates were saved with stored_calories equal
to total kilocalories (55,000 being the "healthy" baseline).

Now, stored_calories is in units of small-c calories, being 55,000,000
for the healthy baseline. This led to a bug CleverRaven#46995 where new characters
created from older-version templates would spawn with too few calories,
by a factor of 1,000.

Detect this when templates are loaded, and multiply stored_calories if
its value appears to be way too small.
wapcaplet added a commit to wapcaplet/Cataclysm-DDA that referenced this pull request Feb 13, 2021
Before CleverRaven#46906, character templates were saved with stored_calories equal
to total kilocalories (55,000 being the "healthy" baseline).

Now, stored_calories is in units of small-c calories, being 55,000,000
for the healthy baseline. This led to a bug CleverRaven#46995 where new characters
created from older-version templates would spawn with too few calories,
by a factor of 1,000.

Detect this when templates are loaded, and multiply stored_calories if
its value appears to be way too small.
wapcaplet added a commit to wapcaplet/Cataclysm-DDA that referenced this pull request Feb 13, 2021
Before CleverRaven#46906, character templates were saved with stored_calories equal
to total kilocalories (55,000 being the "healthy" baseline).

Now, stored_calories is in units of small-c calories, being 55,000,000
for the healthy baseline. This led to a bug CleverRaven#46995 where new characters
created from older-version templates would spawn with too few calories,
by a factor of 1,000.

Detect this when templates are loaded, and multiply stored_calories if
its value appears to be way too small.
@actual-nh
Copy link
Contributor

std::vector<std::pair<time_point>, int>
Every five minutes when food is digested, add an entry of { calendar::turn, intake }.
Then, either have the vector sorted, or just walk through it an delete everything that's too old.
Just sum the entire vector for intake.

I never got back to you - sorry! Thank you for the info; on considering the matter, I actually think a smoother one than that would work better (no abrupt change 24 or 48 hours exactly after eating).

@anothersimulacrum
Copy link
Member Author

std::vector<std::pair<time_point>, int>
Every five minutes when food is digested, add an entry of { calendar::turn, intake }.
Then, either have the vector sorted, or just walk through it an delete everything that's too old.
Just sum the entire vector for intake.

I never got back to you - sorry! Thank you for the info; on considering the matter, I actually think a smoother one than that would work better (no abrupt change 24 or 48 hours exactly after eating).

I think we could certainly make the first work smoothly by making each entry older than say, 24 hours lose 1% (from the initial value) each 15 minutes, then cull it when it hits 0%.

I didn't look too strongly into your recommended solution, so I don't know how complex it would be to implement, but I like the simplicity of this one.

Ramza13 pushed a commit to Ramza13/Cataclysm-DDA that referenced this pull request Apr 12, 2021
Before CleverRaven#46906, character templates were saved with stored_calories equal
to total kilocalories (55,000 being the "healthy" baseline).

Now, stored_calories is in units of small-c calories, being 55,000,000
for the healthy baseline. This led to a bug CleverRaven#46995 where new characters
created from older-version templates would spawn with too few calories,
by a factor of 1,000.

Detect this when templates are loaded, and multiply stored_calories if
its value appears to be way too small.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Items: Food / Vitamins Comestibles and drinks Mechanics: Character / Player Character / Player mechanics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants