microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.36k stars 3.14k forks source link

.Net: Inconsistent Temperature data type in PromptExecutionSettings classes #8541

Open akordowski opened 1 week ago

akordowski commented 1 week ago

Working with SK it came to my attention that the data type of the Temperature property in PromptExecutionSettings classes is inconsistent. Some have the data type float others have double.

Is that intentional? When not wouldn't it make sense to have the same data type in all PromptExecutionSettings classes? As the expected values are mostly in range of -1.0 to 1.0 it would make sens to use the float data type (float 32bit vs double 64 bit). If wished I could provide PR for this issue. Thank you for consideration.

float

double

evchaki commented 1 week ago

@akordowski - thanks for reaching out on this. Each connector has its own data type and matches the type by the underlining service. We abstract on top of these.

akordowski commented 1 week ago

@evchaki Thank you for your reponse.

Each connector has its own data type and matches the type by the underlining service. We abstract on top of these.

After investigating the source code this statement is not quite true. For the OpenAIPromptExecutionSettings class the double values are cast from double to float as you can see below:

https://github.com/microsoft/semantic-kernel/blob/642691158c17c74499039c8c87867cebff042781/dotnet/src/Connectors/Connectors.AzureOpenAI/Core/AzureClientCore.ChatCompletion.cs#L39-L50

And for the GeminiPromptExecutionSettings and MistralAIPromptExecutionSettings classes the values are converted to JSON strings. As we know that the values are at so low range that we don't have to expect an overflow, I would recommend to streamline the data type for a cleaner design. It may not have not that big impact on memory, but why to cast double to float when there is no explicit necessity for it. What do you think?