ibu in whirlpool #418
ibu in whirlpool #418
Conversation
|
Sorry, I misunderstood your comments. I saw many comments not related to my modifications so I understood that I should start again. But it is ok, there is a expression that explains what happened:
I have a comment, related to the following phrase:
The quantity name, according to International System of Units, is length, not distance. Should I really keep distance ? Marcelo |
|
Thanks Marcel! Sorry for the misunderstanding, the UI changes I think would help, but aren't necessary for this pull.
That's surprising to me that isu uses length instead of the dimensionless qty of distance, but not a problem. I'm not concerned with that too much, it's not really an issue. |
|
Doesn't look like the travis CI errors are related to this PR. They seem to either be due to mashwizard.cpp, or some part of Qt. |
|
@pricelessbrewing The travis CI error has something to do with QT. We have seen it before so is not related to the pull request. |
|
I'm getting some errors when building.
|
|
@marcelobarrosalmeida or @dpettersson would one of you please try and build from this PR? I'm unable to perform tests due to error log above. |
|
I can try building it tonight.
Den tis 25 sep. 2018 04:45Mark Price <[email protected]> skrev:
… @marcelobarrosalmeida <https://github.com/marcelobarrosalmeida> or
@dpettersson <https://github.com/dpettersson> would one of you please try
and build from this PR? I'm unable to perform tests due to error log above.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#418 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALLp4sxVVC4aKeJ8fYl-Wm429evYm4qGks5ueZixgaJpZM4VxWLL>
.
|
|
I downloaded a new copy of the develop-branch and applied this commit. It built as it should. |
|
Alright, so I got this built. Some quick feedback from testing below: The issue I'm seeing is that hop time for whirlpool additions don't matter. They should. Additionally, an error should be thrown if the hop addition whirlpool time exceeds the whirlpool time in the equipment editor, or set it so you can't enter a value greater than what is in the equipment. Additionally I'd like to see the tooltip in the equipment editor specify the assumptions made for this formula (ie No chilling or heat applied during whirlpool for the time specified). Lastly, this additional IBU contribution should be applied to boil additions as well, as these oils will continue to isomerize. (see blue graph sections for graphs 4, 5, and 8) on. I'd also like the toggle for this formula to be placed in tools/options/formulas with a dropdown selection for something like "mIBU - modification of Tinseth's approximation." Personally I think this is a great idea and something I really want to see done but probably push back to v2.5, and also implement a new process step for boil/whirlpool and chilling so this can be done properly. |
|
Hi Mark, sorry for the delay .
Em qua, 3 de out de 2018 às 13:23, Mark Price <[email protected]>
escreveu:
Alright, so I got this built. Some quick feedback from testing below:
The issue I'm seeing is that hop time for whirlpool additions don't
matter. They should. Additionally, an error should be thrown if the hop
addition whirlpool time exceeds the whirlpool time in the equipment editor,
or set it so you can't enter a value greater than what is in the equipment.
Yes. I tried to solve this question but I couldn't understand how. In fact,
I believe we could use the bigger WP time inserted in hop additions table
as WP time. And remove this time from equipment. But, again, I couldn't.
Lastly, this additional IBU contribution should be applied to boil
additions as well, as these oils will continue to isomerize. (see blue
graph sections for graphs 4, 5, and 8) on.
It is applied when WP is active. Check again, recipe.cpp:2147
I'd also like the toggle for this formula to be placed in
tools/options/formulas with a dropdown selection for something like "mIBU -
modification of Tinseth's approximation."
But we are not changing how it is done at boil or before. And we do not
have two options at this moment for WP. Unless you decide to add the BS
way (some percentage value of Tinseths), how about to go without options ?
See you
Marcelo
…--
-----------------------------------------------
De perto ninguém é normal.
|
|
Thanks for the correction, I see that the IBUs do in fact change for boil additions when the option is checked in equipment. I'm still not comfortable approving this without the whirlpool time for a hop addition being taken into account. I think it will cause confusion with the user base to have an option for whirlpooling utilization, but not take the hop addition into account. I'd have to take a look at what formula you're using more closely to suggest an approach to take, but in general the whirlpooling utilization for each addition can be taken separately, then summed at the end, and for whirlpool additions set the whirlpooling utilization time equal to max( equipment_time, hop_addition_whirlpool_time). Of course @Brewtarget/integration get the final say if they want to overrule me. |
|
Mark
If you point me what to do for going through the hop list, searching for
time and methods, I believe I can change the kettle dialog and only the
diameter will stay there. If we have WP in some position in the hop list,
we can apply the new formula and use the longest WP time. I was not
comfortable at that point of the code.
Marcelo
Em qua, 3 de out de 2018 às 23:16, Mark Price <[email protected]>
escreveu:
… Thanks for the correction, I see that the IBUs do in fact change for boil
additions when the option is checked in equipment.
I'm still not comfortable approving this without the whirlpool time for a
hop addition being taken into account. I think it will cause confusion with
the user base to have an option for whirlpooling utilization, but not take
the hop addition into account.
I'd have to take a look at what formula you're using more closely to
suggest an approach to take, but in general the whirlpooling utilization
for each addition can be taken separately, then summed at the end, and for
whirlpool additions set the whirlpooling utilization time equal to
max( equipment_time, hop_addition_whirlpool_time).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#418 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACHwrmcUv3qpMGclpr6L9uAwPZ5MoFpcks5uhW-ZgaJpZM4VxWLL>
.
--
-----------------------------------------------
De perto ninguém é normal.
|
|
@marcelobarrosalmeida I think the simplest way to go would be to change the double whirlpool_minutes to be something like min(equipment_whirlpool, hop_addition_whirlpool) so that if a hop addition's whirlpool time exceeds the equipment whirlpool time then it's doesn't continue to add IBUS, but if it's less than the whirlpool time it behaves according to the hop table. If we can do that, then I'd be happy to approve it after testing compared to the alchemyoverlord mIBU calculator. Would also appreciation some code comments in this PR, specifically in IBUMethods.cpp getIbusWhirlpool function. Would still LIKE to move towards separating processes and equipment, but I don't think I'm familiar enough with the code to figure that out right now. It can wait till the next release. |
Hi, here are the changes for supporting better IBU estimation in whirlpool, after flameout. As far as I understood, I did all the changes:
The code does not affect any older recipe. Calculations are only applied if the feature is selected in equipment editor. I only couldn't understand what to do for locales.
About the last attempt of pull request:
As the error reported by travis does not happen in my base I decided to update the code from develop and tried to rebase. But I had many problems during merge. Please, cancel that push request.