Skip to content

[WIP] Automatic overload generator #275

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

Closed
wants to merge 3 commits into from
Closed

[WIP] Automatic overload generator #275

wants to merge 3 commits into from

Conversation

eriove
Copy link
Contributor

@eriove eriove commented Aug 24, 2017

As discussed in #260 here is a proof of concept where I generate operator overloads from a vector of SI quantity exponents.

Right now I'm adding the exponents in the tests but they would be in the JSON files once everything is complete.

Also have some issues with paths, new project formats and nCrunch to sort out before this builds out of the box (no GUI support for symbolic links)

Have not even started to think about the PowerShell scripts but I don't think that will be a problem.

@angularsen
Copy link
Owner

Nice! This looks exciting, I think I see where you are going with this.
You got the unit conversion going, but I don't yet see how the multiplier/value will be included.

Some initial thoughts:

  • How do we represent 5 meters?
  • How do we represent 5 kilometers?
  • Will this replace our current approach of FromBaseUnitFunc and ToBaseUnitFunc in JSON? If so, will we now define SI array values per unit + a multiplier (5 centi/ 10 kilo/1 mega etc)? Will we still define a base unit to store internally or can we now generate conversions between all units based on their SI arrays? That would be awesome!
  • Can we bundle this information in Length, so that Length.Centimeters has an SI array that denotes centimeters, not its base unit meters? On that note, the unit it was created with could also be stored so that say ToString() favors the value and unit it was created with rather than the base unit.

@eriove
Copy link
Contributor Author

eriove commented Aug 26, 2017

This is only for adding operator overloads. The existing JSON-files would need the new array and also a property that points out the base SI-unit when it isn't the base unit (most units that has the automatic milli, centi, deci etc.)

Once I can generate the operator overloads from with the DLL my idea is to create a new Powershell script that calls the DLL and generates C# code with the operator overloads. The end result would be exactly what me and Ulf did manually more than a year ago for some of the units. The main advantage is that it would extend to all units and is less error prone.

It also opens up the possibility to add more intermediate units in an easy way (such as m^-1 or s/m).

But that is for the future. My main reason for doing this is that I really wanted to do it this way back when we did it manually, but I couldn't figure out how before I saw @phatcher's post (in #260) and understood how it could be done.

With some clever work it might be possible to replace the FromUnitToBaseFunc and FromBaseToUnitFunc but I hadn't thought of that before I read your reply

@angularsen
Copy link
Owner

Alright, I'll be watching this eagerly :-)

@eriove
Copy link
Contributor Author

eriove commented Sep 7, 2017

@anjdreas and @aignas this doesn't build but the code generation works and it should show approximately where I'm going

The PowerShell code will require heavy refactoring once the generated code looks good, learning PowerShell by doing this...

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Just some general comments after looking through the work so far. Good stuff!

Assert.Equal(2,actualQuantities.Length);

Assert.Contains(speedQuantity.Name, actualQuantityNames);
Assert.Contains(timeQuantity.Name, actualQuantityNames);
}

[Fact]
public static void ShouldReturnLengthWhenGivenSpeed()
Copy link
Owner

Choose a reason for hiding this comment

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

Test names confused me a bit, I think it would be easier to read if you include the method you are testing, such as GetMultiplicationOverloads_ReturnsLength_GivenSpeed.

Also just a tip, if you have many tests for GetMultiplicationOverloads, you can add a nested class named GetMultiplicationOverloads and put the test methods inside it so they don't have to include it in their name. At least with R# test runner, it becomes a hierarchy of tests and it's clear what tests belong where.

@@ -22,6 +22,21 @@ public static int[] ElementwiseSubtract(this int[] left, int[] right)
return result;
}

public static int[] ElementwiseAdd(this int[] left, int[] right)
Copy link
Owner

Choose a reason for hiding this comment

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

You can use LINQ's .Zip(), like so

	public static int[] ElementwiseAdd(this int[] left, int[] right)
	{
		return left.Zip(right, (l,r) => l + r).ToArray();
	}

However, it just picks the shortest length of the two, so you may want to keep the length check.

foreach (var multiplication in multiplications)
{
yield return multiplication;
yield return new Overload(multiplication.Result, multiplication.Right, multiplication.Left);
Copy link
Owner

Choose a reason for hiding this comment

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

Is this an intended duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was intended, but is wrong. My thought was that multiplication is communicative, but the other multiplication will end up in the other type.

@@ -78,5 +78,6 @@
}
]
}
]
],
"SiArray": [ 1, 0, -1, 0, 0, 0, 0 ]
Copy link
Owner

Choose a reason for hiding this comment

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

I'm just trying to understand still, so you're adding the SI array to each quantity, which represents the base unit?
In the case of speed, it's m/s. And you use this to automatically generate all possible * and / operators across quantities, such as Length / Time = Speed.

Neat!

Thinking further, can we use this to

  1. Convert to other speed units? Use FromBaseToUnitFunc or do we add an SiArray to each unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm considering creating the SI-arrays by parsing the unit names (or abbreviations), but I'm not sure if that is actually faster than just creating the arrays by hand.

It would be possible to create a similar double array with conversion factors for other unit systems (like Imperial). That could be used to automatically generate the code for conversion to that unit system. Since we already have Imperial for most quantities I'm not sure if it is really necessary. But it would be nice when adding new quantities.

@angularsen
Copy link
Owner

I don't mean to nag, but curious if you plan to make progress on this? :-)

@eriove
Copy link
Contributor Author

eriove commented Dec 4, 2017

I plan to continue but it has been too much at work lately so I haven't had time

@angularsen
Copy link
Owner

@eriove I'll close this PR to keep the issue list tidy, but please just reopen it whenever you pick it up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants