Skip to content
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

String formatting of Temperature, Pressure and future physical quantities #1047

Closed
pgrawehr opened this issue Apr 20, 2020 · 27 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio

Comments

@pgrawehr
Copy link
Contributor

While working on the WeatherHelper stuff, I found that it was pretty inconvenient that the Temperature struct did not have a ToString() member. So I added one, and also added a public string ToString(string formatArgs, IFormatProvider culture) method, that takes formatting arguments to print Temperature in different units.

With PR #1046 it is now proposed to do the same for Pressure, but there are a lot of possible ways one may want to format such values, so we need a general concept on how to create such formatting members and what kind of formatting options should be supported.

As a reference, the two proposed ToString() functions for Temperature and Pressure:


        /// <summary>
        /// Returns the string representation of this pressure, with the given format string and using the given culture.
        /// Valid format specifiers are:
        /// PA: Pascal
        /// MBAR: Millibar
        /// KPA: Kilopascal
        /// HPA: Hectopascal
        /// INHG: Inch of mercury
        /// MMHG: Millimeter of mercury
        /// An extra number defines the number of decimal digits to use (default 1)
        /// <example>
        /// <code>
        /// var s = p.ToString("PA2"); -> "101325.01 Pa"
        /// var s = p.ToString("HPA2"); --> "1013.25 hPa"
        /// </code>
        /// </example>
        /// </summary>
        /// <param name="formatArgs">Format string</param>
        /// <param name="culture">Culture to use. Affects the format of the number.</param>
        /// <returns>String representation</returns>
        public string ToString(string formatArgs, IFormatProvider culture)

        /// <summary>
        /// Returns the string representation of this temperature, with the given format string and using the given culture.
        /// Valid format specifiers are:
        /// C: Degrees celsius
        /// F: Degrees Fahrenheit
        /// K: Degrees Kelvin
        /// An extra number defines the number of decimal digits to use (default 1)
        /// <example>
        /// <code>
        /// var s = t.ToString("K2"); -> "293.36°K"
        /// var s = t.ToString("C"); --> "20.21°C"
        /// </code>
        /// </example>
        /// </summary>
        /// <param name="formatArgs">Format string</param>
        /// <param name="culture">Culture to use. Affects the format of the number.</param>
        /// <returns>String representation</returns>
        public string ToString(string formatArgs, IFormatProvider culture)

Questions include:

  • Should such formatting members generally be included or not?
  • How would one specify the unit?
  • Do we need to make the space between unit and number changeable?
  • May the unit name be culture-dependent? (I don't know how a temperature is written in China)

I guess that including a standard formatting for the parameterless ToString() is less disputed and should be done in either case.

CC: @MarkCiliaVincenti @krwq

@pgrawehr pgrawehr added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 20, 2020
@krwq
Copy link
Member

krwq commented Apr 20, 2020

cc: @tarekgh

@krwq
Copy link
Member

krwq commented Apr 20, 2020

Some open questions:

  • how would user remove space between unit and number?
  • are units translated/spelled the same in all languages?
  • are there languages (i.e. some where you type right to left) where number would go second, if so how would user swap the order?

@krwq
Copy link
Member

krwq commented Apr 20, 2020

Also:

  • How would user specify if they prefer to display degrees character?
  • [future thinking even if out of scope for now] How would user specify if they prefer to display full unit name? How would it be translated? (ICU might support that)
  • Should ToString() figure out the default unit for the current culture and use it instead of our default?

@krwq
Copy link
Member

krwq commented Apr 20, 2020

If possible we should reuse NumberFormatInfo here, perhaps this should work very similar to currency formatting (per @tarekgh offline suggestion):
https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings#the-currency-c-format-specifier

Note we can also propose a feature in NumberFormatInfo if we can't re-use it right now

@tarekgh
Copy link
Member

tarekgh commented Apr 20, 2020

I support taking advantage of NumberFormatInfo even it lacks support for temperature and Pressure which we may add later to the framework. but at least we can use it for format the number (using the separators and decimal points) and we always append the unit at the end for now.

@MarkCiliaVincenti
Copy link
Contributor

I'm actually proud I opened a can of worms :) I will give my feedback later.

@pgrawehr
Copy link
Contributor Author

If internally using ToString("F"), NumberFormatInfo is being used implicitly. The implementation already properly decides between . and , for instance.

@tarekgh
Copy link
Member

tarekgh commented Apr 20, 2020

If internally using ToString("F"), NumberFormatInfo is being used implicitly. The implementation already properly decides between . and , for instance.

Yes, you are right. I was hoping not introducing special format inside Temperature and Pressure types and have the formatting standardized in the cultures (in NumberFormatInfo) as we do for currency and other numbers.

@krwq
Copy link
Member

krwq commented Apr 20, 2020

@tarekgh there are some unit multipliers which might be problematic without being a bit more explicit, i.e. Hectopascal vs Pascal - depending on the usage you might or not want the multiplier but inch vs mm of hg/K or C on the other hand is something which I agree can be related to culture

@tarekgh
Copy link
Member

tarekgh commented Apr 20, 2020

there are some unit multipliers which might be problematic without being a bit more explicit, i.e. Hectopascal vs Pascal - depending on the usage you might or not want the multiplier but inch vs mm of hg/K or C on the other hand is something which I agree can be related to culture

The unit shouldn't specified in the format :-) if cultures support temperature and pressure units, then users can pass the culture to get the format for the matched units as desired. think about it as how we handle currency today.

@pgrawehr
Copy link
Contributor Author

What krwq means: For some quantities, the default unit is clear. For temperature, it's Fahrenheit in the US and Celsius everywhere else. But for others it's not so clear: For pressure, it depends on context whether you want mmHG/inchHG or Bar/Pascal/Hectopascal. Or for speed, it might normally be Kilometers/hour vs Miles/hour, except if you are into aviatics or shipping, where it would be knots. And for physicans, it's meters/second (which is the SI unit for it).

@pgrawehr
Copy link
Contributor Author

Related: #869 and the question, which units (or actually more precise: which physical quantities) shall have their own class representation.

@krwq krwq added this to the vNext milestone Apr 21, 2020
@krwq
Copy link
Member

krwq commented Apr 21, 2020

Marking as vNext. At minimum we should figure out if we want to pull out whatever has already been added or not.

@MarkCiliaVincenti
Copy link
Contributor

One thing to keep in mind is that once these are finalized we should update all the samples.

@MarkCiliaVincenti
Copy link
Contributor

MarkCiliaVincenti commented Apr 23, 2020

Also, if we're going to do these, we might want to discuss creating a new unit for distance, which can be used as the return type of WeatherHelper.CalculateAltitude, for example. Currently, it returns meters, but maybe someone wants it in feet or yards.

I'm surprised Microsoft doesn't have a standalone library for these units which could be used by other libraries. They seem almost out of scope in IoT.

@krwq
Copy link
Member

krwq commented Apr 23, 2020

I'd start with designing formatters but have other units in mind when designing instead of adding them in one go.

Note: to move this forward we need exact proposal of how formatters will work including plan for future units. We need to start with investigating NumberFormatInfo and if can it be currently extended and if not how can we make it extendable. Honestly unless it's already possible to extend it I suspect we won't add these formatters in time for vNext since we won't be able to use the APIs until we ship (although we can hack it or partially re-implement as a temporary solution)

@pgrawehr
Copy link
Contributor Author

Note: I believe we should be consistently talking of "(physical) quantities" when we talk about Temperature, Pressure, Distance or Speed, and Units if we talk about meters, degrees or Miles per Hour. This reduces confusion. Each quantity can be represented by different units, but a specific unit may in practical applications represent different quantities (i.e. in aviatics, the default unit for the horizontal distance is nautical miles, while for the vertical distance it is feet).

That said, I think we should have support for at least the following quantities:

  • Temperature
  • Pressure
  • Distance
  • Speed
  • Acceleration
  • Force
  • Direction (Angle)
    And eventually (These are often used with Iot devices, but are not known to be used with different units)
  • Voltage
  • Current

@krwq
Copy link
Member

krwq commented Apr 24, 2020

perhaps we should refrain with this issue for now (and revert all formatters for now) and focus on adding more units. Once we got those then we can focus on formatters and other utilities (perhaps for conversion or some operators to convert between units)

@pgrawehr
Copy link
Contributor Author

I've got some of those prepared already, I can provide a PR maybe tomorrow. Otherwise, I'm fine with removing that for now. Would you remove the standard ToString() as well?

@krwq
Copy link
Member

krwq commented Apr 24, 2020

@pgrawehr cool! Please do PR per unit though. For now I'd start with returning anything in ToString although I think we should put some kind of comment in the xml doc that the behavior may change in the future

@MarkCiliaVincenti
Copy link
Contributor

Does it make sense to have them in the IoT library though, or should we have them elsewhere (ideally in a .NET Standard library)? For example I was working on a completely separate project and wanted to use the Temperature unit.

@pgrawehr
Copy link
Contributor Author

@MarkCiliaVincenti You might be right here. With the ease of creating (and using) new nuget packages, packages should be small. So this should probably go into its own package (i.e. System.Physics) or so. This doesn't necessarily mean we can't keep it in this repo, though (or even that this couldn't be moved/refactored later). But it might be good not to use the Iot.Devices namespace for these. I do like System.Physics, since that would keep things open for stuff like physical calculations (based on those objects) etc.

@JTrotta
Copy link
Contributor

JTrotta commented Apr 24, 2020

Have you ever used this library UnitsNet ?
It's complete and works very well, I used it for years, instead of reinventing the wheel.
It's just a suggestion.

@pgrawehr
Copy link
Contributor Author

Well, I didn't know this, but it does look exactly like what we need. It even has operators to compute i.e. Speed out of distance and time. The license is also matching. And it has formatting members, too 👍

@krwq
Copy link
Member

krwq commented Apr 24, 2020

@JTrotta @pgrawehr I'm not 100% sure - I'd personally like that but there are also questions about servicing in case we ever have a security bug which is in that library. There are some protocols we have to follow as Microsoft which is hard to enforce on external packages. I will need to ask around. @joperezr any clues?

@krwq
Copy link
Member

krwq commented Apr 24, 2020

Let's move conversation about units to #869

@joperezr joperezr added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label May 7, 2020
@pgrawehr
Copy link
Contributor Author

Closing this, as the move to UnitsNet removes the burden of the formatting problem from our library. Opened angularsen/UnitsNet#786 and angularsen/UnitsNet#787 for some findings over there (maybe more to come, as we start using it).

@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio
Projects
None yet
Development

No branches or pull requests

6 participants