search

microsoft / calculator

Windows Calculator: A simple yet powerful calculator that ships with Windows
MIT License
29.64k stars 5.37k forks source link

Proposal to modernize the architecture of Unit Conversions #379

Open rudyhuyn opened 5 years ago

rudyhuyn commented 5 years ago

Problem Statement

There are two flaws with the way the calculator currently manages unit conversions:

Here is a proposal to improve how the calculator manages unit conversions, and for clearing the path for new features going forward.

This proposal is a work-in-progress, some of the changes will lead to a big refactoring of a part of the application. Before even starting to modify how unit conversion works, let's list all the future needs we will have, to be sure to suit all of them and let's debate about the different actions in comments. Don't hesitate to share your ideas and feedback, this document will be regularly updated to take into account your suggestions.

Goal

This proposal is a work-in-progress, some of the changes will lead to a big refactoring of a part of the application. Before even starting to modify how unit conversion works, let's debate and list all the future needs we will have to be sure to suit all of them and let's debate about the different actions in comments. Don't hesitate to share your ideas and feedback, this document will be regularly updated to take into account your suggestions.

Proposal

Step 1: Simplify Data Loader + merge Unit and UnitData.

When the user opens one of the converters, UnitConverterViewModel creates a UnitConverter object, hosting 2 Data loaders: UnitConverterDataLoader and CurrencyConverterDataLoader. To simplify our analysis, we can ignore the second data loader, but there are still many issues from the first data loader.

The UnitConverterDataLoader hosts 2 main objects:

Even if these 2 collections are static and hosted by the same class, they have data redundancy, with the association Category↔Unit being defined by the map<CategoryId, Unit> but also by the UnitData.

The only valuable information hosted by UnitData is Factor and we have no reason not to merge Unit and UnitData and create a single object containing all the necessary information to fully describe a unit:

Unit
Unit Id
Name (string)
Abbreviation (string)
Factor (double)
Offset (double)
AddOffsetFirst (bool)

Step 2a: Get rid of the ratio matrix

When the users open one of the unit converters, UnitConverterDataLoader will, for each category, create a 2D table of all unit combinations from the same category (including pairs of the same unit but also ratio per pairs A->B, B->A) and will calculate the ratio between the 2 units.

For unit conversations, 2996 ratios will be calculated (including 1369 for the category Data) and stored.

When the currency converter is opened or when the user refreshes the currency rates, the application calculates and stores 16,384 ratios, while the user will probably use only 1 or 2 of them.

A ratio is stored by a struct ConversionData, containing 2 doubles and boolean, so a total of 17 bytes for 1 ratio (without counting the overhead of the vector and the map), or 23KB for units and 278KB for currencies!

Now that the calculator is open-sourced, new units will be added, increasing the number of ratios in a category by (Number of units+1)*2 -1).

(very simplified (and not UML compliant) diagram of the current architecture) image_preview

Contrary to UnitConverterDataLoader, values of CurrencyConverterDataLoader can be refreshed by the users, again creating a full matrices of 16K objects each time the user clicks on the update rate button.

Based on the high number of calculations (and memory) necessary to build this matrix, this “optimization” doesn't seem to be one, especially when the conversion from one unit to another is a simple multiplication followed by a division (+ 2 additions if Offset is used), so all these efforts don’t seem to necessary.

Getting rid of these caches will have a direct impact on performance and memory usage.

Step 2b: UnitConverter shouldn’t clone data from the 2 data loaders.

Even if UnitConverter has access to data from UnitConverterDataLoader and CurrencyConverterDataLoader, UnitConverter will create and host a full copy of the ratio matrix, categories, and list of units in a category from the 2 data loaders, multiplying the space used by 2.

Instead, UnitConverter should directly use the data from the data loaders and act as a Facade between the ViewModel and the DataLoaders, saving a lot of space and not forcing a useless sync of caches when users refresh currency rates.

Step 3: Units conversions should manage non-linear unit conversions

The application currently only supports linear conversions (ax + b), making it difficult to support some new units.

Some examples of non-linear units:

We already face this issue with Temperature (Celsius, Fahrenheit, …) forcing us to make some exceptions in the application with custom codes.

The following ticket https://github.com/Microsoft/calculator/issues/347 asks for the support of interval of temperatures, instead of adding more exceptions, we should redesign how the unit conversation between units work.

Instead of using 2 doubles: factor and offset, we should use a pivot unit for each category (already the case, but we must explicitly name the unit used) and add 2 functions to Unit to convert values from and to this pivot unit.

Unit
int Id
string Name
string Abbreviation
double FromPivotUnit(double valueInPivotUnit)
double ToPivotUnit(double value)

The conversion from one unit to another will be possible via the following code:

ToUnit→FromPivotUnit(FromUnit→ToPivotUnit(fromValue))

Some examples:

Length (pivot unit: millimeter) - linear conversion

Centimeter

Meter

Temperature (pivot unit: Celsius) - non-linear conversion

Celsius

Fahrenheit

To simplify the code (and because 99% of units will simply use a factor), we can provide a class UnitUsingFactor, inheriting from Unit and implementing FromPivotUnit and ToPivotUnit.

This solution will allow us to support all types of units without excessive effort and restrictions.

Step 4: Add support of multi-part units

The application currently only supports units with a single number. The 2 tickets #243 and #57 require the application to support units with sub-parts. While the pivot unit can still be a double, values should be represented with a double[]:

Unit
int Id
string UnitName
int NumberOfParts
string[] PartNames
string[] PartAbbreviations
double[] FromPivotUnit(double valueInPivotUnit);
double ToPivotUnit(double[]);

Example for Feet+Inches in the category Length (pivot unit: millimeters)

Unit
int Id
string UnitName “Feet+Inches”
int NumberOfParts 2
string[] PartNames [“Feet”, “Inches”]
string[] PartAbbreviations [“′”, “\””]
double[] FromPivotUnit(double valueInPivotUnit); return [(int)valueInPivotUnit/304.8, (valueInPivotUnit%304.8)/25.4]
double ToPivotUnit(double[]); return value[0]304.8 + value[1]25.4

The UI will also need to be modified in order to support the display (and the editing) of units with subparts.

Conclusion

Once these changes are complete, the application will be more efficient and will allow opportunities for new scenarios that are currently not possible, but first, let's work all together on the new architecture.

Don't hesitate to give your feedback and debate, this post will be regularly updated to take feedback into account.

MicrosoftIssueBot commented 5 years ago

This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it.

grochocki commented 5 years ago

@danbelcher-MSFT Can you take a look at Rudy's proposal?

danbelcher-MSFT commented 5 years ago

Hey Rudy, I generally agree with your proposal and, in particular, I'm most excited for getting rid of the ratio matrix that is generated. I do have one question.. looking at your proposal for Multi-Part Unit, the data members you proposed were two parallel arrays (PartNames and PartAbbreviations) and a count, NumberOfParts. Instead of managing parallel data, it seems like we should compose them together into one class, e.g.

class MultiPartUnit : public Unit {
protected:
    std::vector<UnitMetadata> m_compositeUnitMetadata;
    ...
}; 

class FeetPlusInches : public MultiPartUnit {
public:
    FeetPlusInches() :
        m_compositeUnitMetadata{{ FeetMetadata(), InchesMetadata() }}
        ...
    {}

    std::vector<std::wstring> GetPartNames() {
        std::vector<std::wstring> names;
        for (auto& metadata : m_compositeUnitMetadata) {
            names.push_back(metadata.Name());
        }
        return names;
    }
    ...
rudyhuyn commented 5 years ago

It was a very simplified version to only demo the idea, but I agree with you, we should have a class UnitMetadata/SubUnit and Unit will host a collection (a simple std::array will be enough), the implementation should look like:

SubUnit
wstring Name
wstring Abbreviation
double Value
Unit Parent
Unit
array< SubUnit >SubUnits
wstring Name
double[] FromPivotUnit(double valueInPivotUnit);
double ToPivotUnit(double[]);
double UpdatePivotUnit(); //used by SubUnit

and XAML-side, it can look like :

<!-- Draft, not even tested -->
<ItemsRepeater ItemsSource="{x:Bind SubUnits}">
  <ItemsRepeater.ItemTemplate>
      <DataTemplate>
          <StackPanel Width={x:Bind UniformWidthAvailableForUnit1, Mode=OneWay}>
                <TextBlock Text="{x:Bind Abbreviation}" TooltipService.Tooltip="{x:Bind Name}" />
                 <CalculationResult Value="{x:Bind Value}"/> 
          </StackPanel>
      </DataTemplate>
  </ItemsRepeater.ItemTemplate>
  <ItemsRepeater.Layout>
      <StackLayout Orientation="Horizontal" ItemSpacing="8"/>
  </ItemsRepeater.Layout>
</ItemsRepeater>
danbelcher-MSFT commented 5 years ago

I'm all in favor of simplifying the UnitConverter logic. I'm approving the issue so it's unblocked for PR. I'm sure we'll figure out changes to the architecture as it develops.

danbelcher-MSFT commented 5 years ago

Hey @grochocki, not sure what next steps are for this refactoring work.

rudyhuyn commented 5 years ago

I can start by creating an issue with step 1 only, then once (and if) step 1 approved and merged, create step2a/b, etc...

danbelcher-MSFT commented 5 years ago

I meant in terms of labeling as Approved, going through the feature process, etc. You can see in the history I added 'triage approved' but the issue bot removed the label because this isn't a bug. So my question is about what the right labels are and how to move the deliverable forward.

grochocki commented 5 years ago

We need to make some adjustments to how @MicrosoftIssueBot handles the triage approved label for non-bugs, but in the meantime, unblocking with temporary approved label.

MovGP0 commented 5 years ago

I'm unsure if 'multi part unit's' are a great idea. It seems like a hack for the limitations of the current interface design.

I'm wondering if a more conversional interface would be a better fit for such scenarios. So the user inputs

convert 3 feet 7 inches to cm

or even faster

(3ft+7in)/cm

and done. No wasting time with clicking buttons, searching through dropdown menus, and filling out forms.

MovGP0 commented 4 years ago

It's a bit complex in WolframScript, because you have to write Quantity to declare units:

image

On the HP Prime pocket calculator and the TI nSpire CAS pocket calculator, you write units by prefixing them with an underscore, so it would be (3_ft+7_in)/(1_cm).

xuhongxu96 commented 3 years ago

Hi, I'm writting a prototype in C#, which currently supports pivot value conversion, multi-unit conversion and custom conversion by implementing the interface.

There is a little difference between my implementation and this proposal about multi-unit conversion. I didn't compose the multiple units as one unit and override the conversion methods. I just do sum/divide/mod calculations on pivot value:

public double ConvertMultiUnitsToPivotValue(params UnitValue[] from)
{
    double pivot = 0;
    foreach (var unitValue in from)
    {
        if (!_converters.TryGetValue(unitValue.Unit, out var converter))
        {
            throw new UnitNotFoundException(unitValue.Unit.ToString());
        }
        pivot += converter.ConvertToPivotValue(unitValue.Value, unitValue.Unit);
    }
    return pivot;
}

public double[] ConvertPivotValueToMultiUnits(double pivotValue, params Unit[] toUnits)
{
    var res = new double[toUnits.Length];
    for (var i = 0; i < toUnits.Length; ++i)
    {
        var unit = toUnits[i];
        if (!_converters.TryGetValue(unit, out var converter))
        {
            throw new UnitNotFoundException(unit.ToString());
        }

        var value = converter.ConvertFromPivotValue(pivotValue, unit);
        if (i < toUnits.Length - 1)
        {
            value = Math.Round(value);
        }
        pivotValue -= converter.ConvertToPivotValue(value, unit);
        res[i] = value;
    }
    return res;
}

Link to the above code Link to test cases for multi-unit conversion

Repo of the prototype is here: https://github.com/xuhongxu96/UnitConverter