Bug 445

Summary: Is the class name Scalar in nstime.h appropriate?
Product: ns-3 Reporter: Craig Dowell <craigdo>
Component: coreAssignee: Mathieu Lacage <mathieu.lacage>
Status: RESOLVED FIXED    
Severity: normal CC: gjcarneiro, mathieu.lacage, ns-bugs, quincy.tse, tomh
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: Change Scalar to Dimensionless
Change Scalar to TimeScaleFactor

Description Craig Dowell 2008-12-11 14:33:03 EST
The doxygen in nstime.h says that:

"The ns3::Scalar, ns3::Time, ns3::TimeSquare, and ns3::TimeInvert classes are aliases for the TimeUnit<0>, TimeUnit<1>, TimeUnit<2> and TimeUnit<-1> types respectively."

The dimensionful aliases make sense, but ns3::Scalar bothers me.  All of these aliases describe scalar variables -- but they are scalar variables with different dimensions.

When I think of scalar variables, I think of scalars, vectors and tensors.  The doxygen implies the Scalar type is designed to "scale time" (which is also a scalar) not to be a scalar variable type (cf vector type).

We do have a class Vector and a class Scalar, but they have nothing to do with each other.  You can't multiply a Vector by a Scalar to do scalar multiplication (a linear algebra operation) and you can't multiply two Vectors together to get a Scalar (an inner product).

I think that using ns3::Scalar as an alias for TimeUnit<0> is confusing.  Consider the code snippet:

  Time estimate, error;
  double gain;
  estimate = estimate + Scalar (gain) * error;

Rhetorical question:  Why bother to convert the double <gain> to a Scalar when gain is a double and by definition a double is already a scalar variable?

I think the name Scalar actually confuses what is going on with the important things (in the above code).  What are the important things?  IMO the dimensional analysis and the precision of the intermediate results.  It seems to me that TimeUnit<0> is really something along the lines of a "dimensionless temporary time variable with sufficient precision to store intermediate results of a Time computation." 

It's not very easy to come up with a good name, and I think TimeUnit<0> is actually better than Scalar, but off the top of my head, how about one of the following as an alternative:

  TimePrecision
  DimensionlessTime
  Dimensionless
  IntermediateTime
  TemporaryTime
  TimeTemp

or, like I said, even

  TimeUnit<0>

FWIW I think something like,

  Time estimate, error;
  double gain;
  estimate = estimate + Dimensionless (gain) * error;

makes much more sense than the existing code seen in the snippet above.
Comment 1 Gustavo J. A. M. Carneiro 2008-12-12 06:32:09 EST
I also think TimeUnit does not make complete sense.  When we discussed these things back then I had proposed the concept of "time dimension", or "time power"[1], but Mathieu came back with TimeUnit instead, and I did not want to further prolong the  discussions that were already too long at the time.

Not that I think this is a big deal.  Certainly not worth breaking the API!

[1] http://mailman.isi.edu/pipermail/ns-developers/2006-November/002554.html
Comment 2 Mathieu Lacage 2009-11-23 09:19:04 EST
Ok, how about introducing DimensionLess and deprecating Scalar ?
Comment 3 Gustavo J. A. M. Carneiro 2010-05-17 07:06:27 EDT
(In reply to comment #2)
> Ok, how about introducing DimensionLess and deprecating Scalar ?

Wikipedia article: http://en.wikipedia.org/wiki/Dimensionless_number

+1 for Dimensionless (single word)
Comment 4 Quincy Tse 2010-05-24 00:20:40 EDT
Dimensionless would be better than Scalar, but might still not make sense if that "Scalar" has other dimensions (kilogram, etc).
Comment 5 Josh Pelkey 2010-07-07 20:14:29 EDT
Created attachment 932 [details]
Change Scalar to Dimensionless

If we are moving forward on this one, we would like to get this one in for ns-3.9.  Since most like Dimensionless, I've attached a patch that changes all Scalar to Dimensionless.  I also changed some lower-case scalar in comments where needed.  Bindings updated also.  I didn't bother with deprecating Scalar.  If there are any more comments, please respond in the next few days.  If there aren't any objections, I'll go ahead and apply.
Comment 6 Tom Henderson 2010-07-08 01:52:39 EDT
(In reply to comment #5)
> Created an attachment (id=932) [details]
> Change Scalar to Dimensionless
> 
> If we are moving forward on this one, we would like to get this one in for
> ns-3.9.  Since most like Dimensionless, I've attached a patch that changes all
> Scalar to Dimensionless.  I also changed some lower-case scalar in comments
> where needed.  Bindings updated also.  I didn't bother with deprecating Scalar.
>  If there are any more comments, please respond in the next few days.  If there
> aren't any objections, I'll go ahead and apply.

I'd like to resolve this one way or the other (FIXED or WONTFIX).

I scanned through the patch and I had to stare at some of the lines for a while until they made sense to me; e.g.

Dimensionless frame = ((timestamp - Simulator::Now ()) / frame_duration);

Part of the problem for me might be that Dimensionless doesn't initially convey that this is some kind of underlying Time data type.  A name like "TimeScaleFactor" might be more meaningful and match with the other typedefs, such as TimeInvert and TimeSquare.  I'm guessing that Scalar was picked originally because it is trying to scale (by a scale factor) a time quantity.

Another possibility would be to do this:

+// Scalar is deprecated; use TimeUnit<0> or TimeScaleFactor instead
typedef TimeUnit<0> Scalar;
+ typedef TimeUnit<0> TimeScaleFactor;
typedef TimeUnit<-1> TimeInvert;
typedef TimeUnit<2> TimeSquare;

and make a pass through the codebase changing "Scalar" to "TimeUnit<0>" (as Craig suggested) or "TimeScaleFactor", and just stop using Scalar in our codebase (but leave in for backward compatibility).
Comment 7 Mathieu Lacage 2010-07-08 04:40:48 EDT
(In reply to comment #6)

> +// Scalar is deprecated; use TimeUnit<0> or TimeScaleFactor instead
> typedef TimeUnit<0> Scalar;
> + typedef TimeUnit<0> TimeScaleFactor;
> typedef TimeUnit<-1> TimeInvert;
> typedef TimeUnit<2> TimeSquare;
> 
> and make a pass through the codebase changing "Scalar" to "TimeUnit<0>" (as
> Craig suggested) or "TimeScaleFactor", and just stop using Scalar in our
> codebase (but leave in for backward compatibility).

I would be fine with the above.
Comment 8 Gustavo J. A. M. Carneiro 2010-07-08 04:58:02 EDT
TimeScaleFactor sounds good.
Comment 9 Josh Pelkey 2010-07-08 09:52:18 EDT
Created attachment 933 [details]
Change Scalar to TimeScaleFactor

Also keeps Scalar typedef for backward compatibility, as Tom suggested.
Comment 10 Mathieu Lacage 2010-07-09 02:44:14 EDT
Thinking some more about this, and since I had to hack on this code a lot this week, I feel that maybe now is the time to go down a more radical path:

1) remove the Time<N> template and replace it with a simple Time class
2) typedef Scalar, TimeInvert, etc. to Time
3) add GetDouble to Time.

The above would allow me to add support for the missing C++ operators we don't support (+=, =+, *=, etc.). Also, I suspect that it would be easier for users to not to have to worry about the Time<N> stuff.
Comment 11 Mathieu Lacage 2010-07-09 06:54:43 EDT
(In reply to comment #10)
> Thinking some more about this, and since I had to hack on this code a lot this
> week, I feel that maybe now is the time to go down a more radical path:
> 
> 1) remove the Time<N> template and replace it with a simple Time class
> 2) typedef Scalar, TimeInvert, etc. to Time
> 3) add GetDouble to Time.
> 
> The above would allow me to add support for the missing C++ operators we don't
> support (+=, =+, *=, etc.). Also, I suspect that it would be easier for users
> to not to have to worry about the Time<N> stuff.

I have pushed a version of that in http://code.nsnam.org/mathieu/ns-3-bug826

It's easy to revert this specific thing if others disagree with it (it's the changeset on top of the tree)

So, now, you can write things like:

Time foo = MicroSeconds (1000) * 10.5;
Time foo = MicroSeconds (1000) * 10;
Comment 12 Tom Henderson 2010-07-12 00:47:51 EDT
> I have pushed a version of that in http://code.nsnam.org/mathieu/ns-3-bug826
> 
> It's easy to revert this specific thing if others disagree with it (it's the
> changeset on top of the tree)
> 
> So, now, you can write things like:
> 
> Time foo = MicroSeconds (1000) * 10.5;
> Time foo = MicroSeconds (1000) * 10;

This seems nicer from a usability perspective; are there any downsides in your view to changing it this way?  What led you away from this type of design initially?
Comment 13 Mathieu Lacage 2010-07-12 02:47:45 EDT
(In reply to comment #12)
> > I have pushed a version of that in http://code.nsnam.org/mathieu/ns-3-bug826
> > 
> > It's easy to revert this specific thing if others disagree with it (it's the
> > changeset on top of the tree)
> > 
> > So, now, you can write things like:
> > 
> > Time foo = MicroSeconds (1000) * 10.5;
> > Time foo = MicroSeconds (1000) * 10;
> 
> This seems nicer from a usability perspective; are there any downsides in your
> view to changing it this way?  What led you away from this type of design
> initially?

I guess that there are two major changes here:
  i. now, you can multiply with double directly.
  ii. now, there is no compile-time unit checking anymore

regarding ii., I don't remember the details of what led us to the current design but in general, I believe that the only sane way to implement all arithmetic operators is to behave like a normal native type which means, implement our own 'double' type. More about this later.

regarding i., I always argued against asymetric arithmetic operators:

Time operator (const Time &o, double a);

because they force you to implement all possible ordering combinations and it's really hard to make them work reliably in a way which is not surprising to the user. Here, I avoided them and use instead implicit conversion operators which I did not really know how to use before. 

However, this code allows you to write also:

Time foo = MicroSeconds (1000) + 10.5;

which is equivalent to 

Time foo = MicroSeconds (1000) + TimeStep (10.5);

which is neither a common nor a useful idiom.

Anyway, after this last comment, I spent a while thinking about this over the WE and what is increasingly obvious to me is that the current API is confusing:

Time foo = MicroSeconds (10);
Time bar = foo * Scalar (1.5);

what is the value of bar ? is it 1.5 * 1e-6 or 10.0 * 1.5 ?

The key here I think is that we have mixed together 2 features:
  - management of abstraction of time resolution
  - high-precision airhmetic operations

What we probably should have done instead is:

  1) introduce int64x64_t which is a wrapper around the HighPrecision class which implements all arithmetic operators. You can use this type instead of using double. it's really a drop-in replacement.

  2) make Time use int64x64_t

class Time
{
public:
  // note the explicit keyword below
  explicit Time (int64x64_t);
  const int64x64_t &GetValue (void) const;
  const int64x64_t &GetMicroSeconds (void) const;
  const int64x64_t &GetMilliSeconds (void) const;
};

Time Seconds (int64x64_t v);

class int64x64_t
{
public:
  int64x64_t (int v);
  int64x64_t (double v);
  int64_t GetInteger (void) const;
  double GetDouble (void) const;
};

meaningful example: 
Time now = Simulator::Now ();
int64x64_t v = now.GetMicroSeconds () * 10.0;
int64x64_t v = now.GetMicroSeconds () + 10.0;

I fear that such a change would be a pretty major API change though. I did not think through _all_ the details but there is at least one major issue: GetMicroSeconds returns int64x64_t. This would break code which expects a int64_t instead. We could add an implicit conversion from int64x64_t to int64_t to avoid this problem but, I fear that would would wreak havoc in other things.

This needs some more thinking and probably a prototype to flesh out the details.
Comment 14 Tom Henderson 2010-07-21 00:00:54 EDT
reassigning, slipping to ns-3.10
Comment 15 Mathieu Lacage 2011-04-17 08:27:22 EDT
merged ns-3-time branch where Scalar is no more and replace by a special Time constructor that takes an int64x64_t.