Converting between time and frequency is difficult

[apache-hb]

Given this example I would expect a conversion from femtoseconds to hertz, currently this is a compilation error. Is this intended behaviour? Edit: this is possible by dividing 1.0 by f, which works but doesnt work on embedded devices without floating point.

[chiphogg]

You can't (directly) convert a time in femtoseconds to a frequency in hertz, because a time is not a frequency.

What you're looking for is a smart inverse function, i.e., inverse<si::hertz>(f). This function will figure out that we should be dividing the underlying numeric value into $10^{15}$, not into $1$. See https://godbolt.org/z/73bP9dMTx.

...of course, since $10^{15}$ can't be represented in int, mp-units currently produces an incorrect answer of -153 Hz, due to overflow. I think we are missing a compile time check such as this one: ideally, your specific example would produce a hard compiler error, but inverse<target_unit>(q) would be how you'd solve the problem in general.

[chiphogg]

Of course, an alternative way to get the correct answer would be to use an explicit-rep variant of inverse, so you could specify that the computation should take place in, say, int64_t. Here's how to do that in Au, where you'll see that you get the answer you expect. I assume there's an explicit-rep version for mp-units but I wasn't able to figure out what it was.

[mpusz]

@chiphogg, I never thought about adding such an overload for inverse. It might be a good idea, though.

[chiphogg]

Yeah, it's similar to that table we made of all the different ways to spell .in<...>(...) for quantity. I think that table also had value_cast as an alternative. Do you remember the table I'm talking about? I can picture it, but I don't know how to find it. If you find it, link it here --- I think it'd be helpful!

Anyway... adding an API option like this is just part of that same grammar that we were expanding on in that table. We need to find some way to give users a way to tell us which rep to use.

Oh, and we probably also need to add that hard compiler error for the implicit rep case on inputs like these too. 😅

[mpusz]

Do you mean this table: https://mpusz.github.io/mp-units/latest/users_guide/framework_basics/value_conversions/#value-conversions-summary?

Oh, and we probably also need to add that hard compiler error for the implicit rep case on inputs like these too. 😅

Probably yes. Could you please file an issue for this?

[chiphogg]

That looks like a distilled version of the table I had in mind. I did end up tracking down the original. I remembered the original being much bigger, and it is --- but I think that boils down to not including a column for quantity spec on the official doc page. (I think the new table is equivalent to the old table if we keep only rows where qs is "Same".)

I filed #671 to track the hard compiler error.

[chiphogg]

This is listed as completed in 5d4e1aa, but I don't see any new inverse related API in that commit. What is the new API for computing the inverse of 10'000'000 * si::femto<si::second> in si::hertz, without leaving the integral domain?

(I'm reopening for bookkeeping because I'm reasonably confident the linked commit doesn't solve the problem, but if I'm wrong about that, it'll be easy enough to close this issue again.)

[mpusz]

The current code with inverse should now fail to compile instead of providing a wrong result. It is easy to do it right with inverse<si::hertz>(f.in<int64_t>()): https://godbolt.org/z/f4czGeYjr.

I am unsure if we should provide additional overloads for each function that can change the unit.

[chiphogg]

If this is the same thing as the hard compiler error, then why did you ask me to file #671 as a separate issue? 😅

The interfaces are subtly different in principle. inverse<si::hertz, int64_t>(f) has the full context in one place, and applies to the original input. It's the most general interface. inverse<si::hertz>(f.in<int64_t>()) breaks it down into two steps on the user side.

Now, in this specific case, I don't think the two options are meaningfully different in practice. For inverse generally, across all permutations of input/output rep and input/target unit, I'm not sure whether I could cook up an example that would give meaningfully different results. Let's see... let One<T> stand for "a dimensionless quantity whose value is 1, expressed in the appropriate units, using a rep of T". (So, for the ns/Hz case, then One<int64_t> would be a quantity wrapping an int64_t that holds a value of $10^{15}$.)

• Current approach:
  • One<T> / x.in<T>().
• Explicit rep
  • static_cast<T>(One<C> / x), where C is the common type of T and the rep of x.

If you can think of a combination of quantities and types where these are meaningfully different, then we should consider adding the explicit-rep overloads. If we can't, then it's probably fine to just close this ticket again.

But the distinction between the explicit-rep version, and an explicit conversion of the inputs, remains important in general, and we should be asking ourselves these questions for every interface IMO.

[mpusz]

If this is the same thing as the hard compiler error, then why did you ask me to file #671 as a separate issue? 😅

I didn't want to track the overflowing conversion issue here as this issue was clearly about the user's misunderstanding of time and frequency quantities (there is no way to convert between those). This is why I planned to close it after your explanation, leaving the other one open.

[mpusz]

But the distinction between the explicit-rep version, and an explicit conversion of the inputs, remains important in general, and we should be asking ourselves these questions for every interface IMO.

Sure. However, I do not want to explode the overload sets without a very good reason. It would make it harder to standardize. We can always add more overloads if needed, even in the following C++ releases. I would prefer to see compelling use cases from our users first, though. It would make it easier for us to defend additional overloads in the Committee.

[chiphogg]

Makes sense, thanks!