Open EvanKirshenbaum opened 5 months ago
This issue was referenced by the following commit before migration:
Looking at the code, I think it may make more sense to do it fully to refactor cmd_line
to actually have separate objects per dimension. Right now, ArgUnits
is defined using two static maps:
class ArgUnits:
known: Final[dict[type[Quantity], dict[str, UnitExpr]]] = defaultdict(dict)
patterns: Final[dict[type[Quantity], Pattern]] = {}
where all interactions are via class methods that require a type[Quantity]
argument to say which dimension they're talking about. (The exception is units_arg
, which runs through all of ArgUnits.known
).
What I propose to do is to turn ArgUnits
into something like DimArg[Q]
, where DimArg
has a mapping of known instances, one per quantity type, each containing their own units, constants, patterns, and default strings for error messages. With this mapping, we can also do the functionality of units_arg
by querying all of the instances.
If we give DimArg
a __call__(str)
method, we should be able to do something like either
voltage_arg = DimArg[Voltage](units={ ... })
or, possibly better,
voltage_arg = DimArg.for_dim(Voltage)
voltage_arg.add_units({ ... })
This is closer to what's done now, and has the advantage that there can be multiple calls, possibly even in user code. Yeah, I think that's the way to go.
To implement constants, you can just have a mapping from string to quantity (added to by passing in a mapping from quantity to sequence of strings), e.g., {"never": 1000000*day}
that's looked at first in parse_arg()
.
I haven't added the notion of constants, but I have enabled the "Let a task say it should live forever" behavior we actually need for the problem at hand.
I changed things so that default_min_time
can now be None
. In this case, the task should live forever, unless --max-time
(or default_max_time
) is a Time
. I also moved all of these defaults onto the Task
rather than being in Exerciser
, so that Task.default_min_time
can be 0*ms
while DisplayOnly.default_min_time
can be None
.
I further made the logic a bit more flexible by adding --hold-display-for
, which specifies a minimum time to keep the display open once the Task
ends.
Finally, I added logging, so that the user knows when things will go down.
It's really stupid that in order to use
interactive.py
, in order for it not to disappear immediately, you need to specify a--min-time
argument with a fixed, but large value (e.g.,1day
).It would make more sense for
time_arg
to allow something likenever
to return an infiniteTime
(or at least a hugely large one). This will requireArgUnits
to require a second map parallel topatterns
(which maps unit names to unit expressions for a given dimension) holding, for each dimension, a mapping of names to quantities.The default value for
--min-time
, currently a fixed5*minutes
, should instead be taken from theTask
. Note that--min-time
is added inExerciser.add_common_args_to()
, so this will require that this (called inExerciser.setup_task()
) get passed theTask
.Migrated from internal repository. Originally created by @EvanKirshenbaum on Feb 04, 2023 at 3:04 PM PST.