Parsing negative FeetInches values

[deanwiles]

Describe the bug
Unexpected behavior from Length.ParseFeetInches when parsing negative coordinates.

We have an application that imports values on the x- and y-axes relative to a home position. A position that is 11 ft 2 in from the left of center has an x value of -11'2". Note that the negative sign applies to both feet and inches, essentially meaning -(11'2"). Unfortunately Length.ParseFeetInches evaluates -11'2" as -10'-10" (effectively -11' plus 2").

It appears that Length.ParseFeetInches is expecting two negative signs when parsing negative values (e.g. -11'-2"). Is that the correct behavior? I've never seen multiple negatives in coordinates, but I also couldn't find any defined standard for this online.

To Reproduce
Samples from Visual Studio Debugger Immediate Window:
?Length.ParseFeetInches("-11'2"").FeetInches.ToString()
"-10 ft -10 in"
?Length.ParseFeetInches("-11 ft 2 in").FeetInches.ToString()
"-10 ft -10 in"
?Length.ParseFeetInches("-11'-2"").FeetInches.ToString()
"-11 ft -2 in"
?Length.ParseFeetInches("-11 ft -2 in").FeetInches.ToString()
"-11 ft -2 in"
?Length.ParseFeetInches("-(11'2")").FeetInches.ToString()
throws 'System.FormatException'

Expected behavior
I expected that the leading negative sign would apply to the entire unit (e.g. -11'2" = -(11'2")).

Comments
I really like UnitsNet! Very robust and flexible. (Just wasn't expecting this behavior.)

It doesn't appear that Length.ParseFeetInches handles parentheses, so if the current behavior is correct, I may have to detect and strip off leading negatives and then multiply by -1 after parsing.

Thanks, Dean.

[angularsen]

Hi and thanks for a detailed report, very helpful.

My first hunch is that you've run into an edge case we simply didn't think of. Parsing feet-inches has a special implementation to support parsing two parts, and I guess it doesn't recognize the minus for both parts.

If you would be interested in trying to fix the bug and write a new unit test case for this scenario, then I'm happy to assist on getting a pull request merged. I'm currently on vacation and the other collaborators have been busy lately too, so it can take some time to find the time to look closer at this.

[tmilnthorp]

@deanwiles this has been fixed in 4.27.0

[deanwiles]

Thank you @angularsen and @tmilnthorp for such a fast response.

Unfortunately, there are two related issues with the fix.

1. It does not handle the case with a negative position of less than 1 foot. (e.g. -0'1" parses out to 0'1" - positive instead of negative). Try the following unit test with TryParseFeetInches(...):
   [InlineData("-0'1\"", -0.08333333)] // Negative zero feet one inch

The following code handles that case, but you may have a better solution.

    public static bool TryParseFeetInches([CanBeNull] string str, out Length result, IFormatProvider formatProvider = null)
        {
            if (str == null)
            {
                result = default;
                return false;
            }

            str = str.Trim();

            // This succeeds if only feet or inches are given, not both
            if (TryParse(str, formatProvider, out result))
                return true;

            var quantityParser = QuantityParser.Default;
            string footRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Foot, formatProvider, matchEntireString: false);
            string inchRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Inch, formatProvider, matchEntireString: false);

            // Match entire string exactly
            string pattern = $@"^(?<feet>{footRegex})\s?(?<inches>{inchRegex})$";

            var match = new Regex(pattern, RegexOptions.Singleline).Match(str);
            if (!match.Success) return false;

            var feetGroup = match.Groups["feet"];
            var inchesGroup = match.Groups["inches"];
            if (TryParse(feetGroup.Value, formatProvider, out Length feet) &&
                TryParse(inchesGroup.Value, formatProvider, out Length inches))
            {
                if (feet.Value == 0)
                {
                    // If feet numeric value is 0, but feet text value was -0, then make inches negative
                    var numFormat = formatProvider != null
                        ? (NumberFormatInfo)formatProvider.GetFormat(typeof(NumberFormatInfo))
                        : NumberFormatInfo.CurrentInfo;
                    var negativeSign = numFormat.NegativeSign;
                    if (feetGroup.Value.Trim().StartsWith(negativeSign)) inches *= -1;
                }
                result = feet.Value >= 0 ? feet + inches : feet - inches;
                return true;
            }

            result = default;
            return false;
        }

2. The other issue occurs when converting a negative Length to FeetInches. In that case, if feet < 0 then inches should have the absolute value so that there is only one negative value. One additional line of code can handle that.

        public FeetInches FeetInches
        {
            get
            {
                var inInches = Inches;
                var feet = Math.Truncate(inInches / InchesInOneFoot);
                var inches = inInches % InchesInOneFoot;
                if (feet < 0) inches = Math.Abs(inches);    // If feet are negative then inches should not also be negative

                return new FeetInches(feet, inches);
            }
        }

[tmilnthorp]

Indeed that case was missed! I have created a fix in PR #675

As for the double negatives, I believe that's a fine assumption, but maybe not. Would -2 feet -6 inches be the same as -1 foot, 6 inches? Or should we always assume inches to be negative? @angularsen

That's a different issue/PR scope though

[angularsen]

Good catch, and good fix. Merged.

As for double negatives, I'm not quite sure what my intuition expects.
I googled a few calculators, not all of them supported negative values but these did when I tried converting -1m to feet and inches:

• wolframalpha.com neg/pos
• csgnetwork.com neg/neg
• ginifab.com neg/pos
• endmemo.com neg/neg

So something tells me that negative feet/inches is not widely used and there is no single convention on what is right here.

1. Should Parse() support both -1' 2" and -1' -2"? And they should be equivalent?
2. Should internal value representation and ToString() be -1' 2" or -1' -2"?

[deanwiles]

Thanks, @tmilnthorp for the fix in PR #675; much cleaner than my suggestion. However, if a double negative occurs, it does not yield the expected result (e.g. -1'-2" => -(1'-2") => -0'10" instead of -1'2").

        [InlineData("-0 ft -1 in", -0.08333333)] // Inches only (with double negative)
        [InlineData("-1 ft -1 in", -1.08333333)] // (with double negative)

This can be fixed by changing the line that combines feet and inches:

            var negativeSignGroup = match.Groups["negativeSign"];
            var feetGroup = match.Groups["feet"];
            var inchesGroup = match.Groups["inches"];

            if (TryParse(feetGroup.Value, formatProvider, out Length feet) &&
                TryParse(inchesGroup.Value, formatProvider, out Length inches))
            {
                result = Length.FromFeetInches(feet.Feet, Math.Abs(inches.Inches));

                if(negativeSignGroup.Length > 0)
                    result = -result;

                return true;
            }

Thanks, @angularsen for the online examples.
wolframalpha.com treats the feet/inches result as a unit, whereas csgnetwork.com treats the result as the sum of two parts.
Neither ginifab.com nor endmemo.com appear to handle negative lengths because their feet/inches conversions of -1m are incorrect (-4' 8 5⁄8" and -4' -3.370079'', respectively).

I think the answer lies in whether FeetInches is a unit or the sum of two parts?
As a unit, there should be just one leading unary operator (e.g. -1'2", -0'2", -2"). In this case, a second negative shouldn't be used, but if encountered, should imply subtraction of two distinct units (e.g. -1'-2" => -1'2"). Note the current handling of this as mentioned above.
However, if FeetInches are two parts, then you're back to the original behavior where the absence of an operator implies the addition of the two parts (e.g. -1'2" => 0'-10"), which was counter-intuitive to me.

IMHO, FeetInches should be a unit and just parse and output a single unary negative sign (e.g. -1'2", -0'2", -2"). Negative decimal numbers and their fractional parts could be used for precedence. There are differing views of whether the fractional part can be negative, but the decimal number as a whole just has one leading negative sign if < 0.

A way to implement this would be to add a Sign property to FeetInches:

        /// <summary>
        ///     Construct from feet and inches.
        /// </summary>
        public FeetInches(double feet, double inches)
        {
            Feet = feet;
            Inches = inches;
            Sign = feet >= 0 && inches >= 0 ? "" : "-";
        }

        ...

        /// <summary>
        /// The positive "" or negative "-" Sign for the FeetInches unit.
        /// </summary>
        public string Sign { get; }

        ...

        /// <summary>
        ///     Outputs feet and inches on the format: {Sign} {feetValue} {feetUnit} {inchesValue} {inchesUnit}
        /// </summary>
        /// <example>Length.FromFeetInches(3,2).FeetInches.ToString() outputs: "3 ft 2 in"</example>
        /// <param name="cultureInfo">
        ///     Optional culture to format number and localize unit abbreviations.
        ///     If null, defaults to <see cref="Thread.CurrentUICulture"/>.
        /// </param>
        public string ToString([CanBeNull] IFormatProvider cultureInfo)
        {
            cultureInfo = cultureInfo ?? CultureInfo.CurrentUICulture;

            var footUnit = UnitAbbreviationsCache.Default.GetDefaultAbbreviation(LengthUnit.Foot, cultureInfo);
            var inchUnit = UnitAbbreviationsCache.Default.GetDefaultAbbreviation(LengthUnit.Inch, cultureInfo);

            // Note that it isn't customary to use fractions - one wouldn't say "I am 5 feet and 4.5 inches".
            // So inches are rounded when converting from base units to feet/inches.
            return string.Format(cultureInfo, "{0}{1:n0} {2} {3:n0} {4}",
                Sign, Math.Abs(Feet), footUnit, Math.Abs(Math.Round(Inches)), inchUnit);
        }

Associated tests:

        [Theory]
        [InlineData("1'", "1 ft 0 in")] // Feet only
        [InlineData("1′", "1 ft 0 in")] // Feet only
        [InlineData("1\"", "0 ft 1 in")] // Inches only
        [InlineData("1″", "0 ft 1 in")] // Inches only
        [InlineData("0' 1\"", "0 ft 1 in")] // Inches only
        [InlineData("0' 1″", "0 ft 1 in")] // Inches only
        [InlineData("0′ 1\"", "0 ft 1 in")] // Inches only
        [InlineData("0′ 1″", "0 ft 1 in")] // Inches only
        [InlineData("1' 1\"", "1 ft 1 in")] // Normal form
        [InlineData("1′ 1″", "1 ft 1 in")] // Normal form
        [InlineData(" 1′ 1″ ", "1 ft 1 in")] // Normal form, requires trimming
        [InlineData("1'1\"", "1 ft 1 in")] // Without space
        [InlineData("1′1″", "1 ft 1 in")] // Without space
        [InlineData("1 ft 1 in", "1 ft 1 in")]
        [InlineData("1ft 1in", "1 ft 1 in")]
        [InlineData("-1'", "-1 ft 0 in")] // Feet only
        [InlineData("-1′", "-1 ft 0 in")] // Feet only
        [InlineData("-1\"", "-0 ft 1 in")] // Inches only
        [InlineData("-1″", "-0 ft 1 in")] // Inches only
        [InlineData("-0' 1\"", "-0 ft 1 in")] // Inches only
        [InlineData("-0' 1″", "-0 ft 1 in")] // Inches only
        [InlineData("-0′ 1\"", "-0 ft 1 in")] // Inches only
        [InlineData("-0′ 1″", "-0 ft 1 in")] // Inches only
        [InlineData("-0 ft -1 in", "-0 ft 1 in")] // Inches only (with double negative)
        [InlineData("-1' 1\"", "-1 ft 1 in")] // Normal form
        [InlineData("-1′ 1″", "-1 ft 1 in")] // Normal form
        [InlineData(" -1′ 1″ ", "-1 ft 1 in")] // Normal form, requires trimming
        [InlineData("-1'1\"", "-1 ft 1 in")] // Without space
        [InlineData("-1′1″", "-1 ft 1 in")] // Without space
        [InlineData("-1 ft 1 in", "-1 ft 1 in")]
        [InlineData("-1 ft -1 in", "-1 ft 1 in")] // (with double negative)
        [InlineData("-1ft 1in", "-1 ft 1 in")]
        public void TryParseFeetInchesToString(string str, string expectedOutput)
        {
            Assert.True(Length.TryParseFeetInches(str, out Length result));
            Assert.Equal(expectedOutput, result.FeetInches.ToString());
        }

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.