FLOPs calculation for Monty #9
Conversation
hlee9212
changed the title
hlee9212
changed the title
|
Hey @scottcanoe, I think Floppy has matured enough to be part of the TBP's |
…tions - Introduced new operations: FloorDivide, Modulo, BitwiseAnd, BitwiseOr, and Power. - Updated FlopCounter to wrap these new operations. - Enhanced ufunc wrapper to map new operations for FLOP counting. - Added a new CSV file to track the frequency of operations used.
…OP counting logic. Addresses: thousandbrainsproject#9 (comment)
…nd floor operations, totaling 2 FLOPs per element. Addresses: thousandbrainsproject#9 (comment)
…of 20 FLOPs per value, enhancing the docstring to clarify the calculation process based on Taylor series expansion. Addresses: thousandbrainsproject#9 (comment)
…eration to use arctan for a total of 44 FLOPs, and revised ArcTangentOperation to estimate 20 FLOPs per value for consistency with sine operation.
…te of 20 FLOPs per value.
…te of 33 FLOPs per value.
…r FLOP counting. Addresses: thousandbrainsproject#9 (comment)
There was a problem hiding this comment.
@nielsleadholm, I was unable to finish reviewing all the changes you highlighted regarding the context manager and the machinery for instrumenting FLOP counting.
This review covers only the following code:
TrackedArrayFlopCounterMontyFlopTracermonty_profiling/frameworks/run.py
I restricted my review and comments to double-checking the desired functionality only.
I won't get to the FLOP versions of learning module, goal state, and object model until next week.
|
|
||
| try: | ||
| # Analyze a directory of Python files | ||
| results = analyzer.analyze_directory("/Users/hlee/tbp/tbp.monty/src") |
There was a problem hiding this comment.
issue: hardcoded user-specific path
| ) | ||
| from floppy.monty_flop_tracer import MontyFlopTracer | ||
|
|
||
| def wrap_monty_with_flops(monty_cls, experiment_cls): |
There was a problem hiding this comment.
issue: monty_cls argument is unused
| return monty_cls, experiment_cls | ||
|
|
||
|
|
||
| def run_with_flops(exp_config: Dict[str, Any]): |
There was a problem hiding this comment.
issue since wrap_monty_with_flops above does not alter monty_cls in any way. All code here related to the monty class can be removed.
| "argmin": (np, "argmin"), | ||
| "argmax": (np, "argmax"), | ||
| } | ||
| self._patch_targets = { |
| self._original_array_func = np.array | ||
|
|
||
| # Override numpy array creation to return tracked arrays | ||
| def tracked_array(*args, **kwargs): |
There was a problem hiding this comment.
I think this is intended to create a named closure and close over the value of self._original_array_func, thus binding it specifically to the value on line 319.
However, instead of a closure, this could be a FlopCounter instance method instead:
class FlopCounter(ContextDecorator):
# ...
def tracked_array(self, *args, **kwargs):
arr = self._original_array_func(*args, **kwargs)
return TrackedArray(arr, self)And used like this:
np.array = self.tracked_arrayOne could argue that what I showed above is less readable as the code would look like:
self._original_array = np.ndarray
self._original_array_func = np.array
np.array = self.tracked_arrayIt appears less obvious that self.tracked_array wraps self._original_array_func
| def _wrap_methods(self): | ||
| """Wrap key Monty methods to count FLOPs.""" | ||
|
|
||
| def wrap_method(method_name, target_obj): |
There was a problem hiding this comment.
issue: This function only closes over self and is not being used as a replacement named function. It should be an instance method instead.
There was a problem hiding this comment.
issue: This function uses order of arguments that is the opposite of getattr and setattr. As the argument order can be anything, aligning with getattr and setattr argument ordering may reduce cognitive load.
|
|
||
|
|
||
| # Helper function to easily add FLOP tracking to any Monty instance | ||
| def add_flop_tracking(monty_instance, experiment_instance): |
There was a problem hiding this comment.
issue: This function is unused.
| "run_episode": self.experiment.run_episode, | ||
| "pre_episode": self.experiment.pre_episode, | ||
| "pre_step": self.experiment.pre_step, | ||
| "step": self.monty.step, |
There was a problem hiding this comment.
issue: This is a Monty method, not a MontyExperiment method. I think this causes a problem further down in _wrap_methods().
| experiment_methods = [ | ||
| "pre_episode", | ||
| "pre_step", | ||
| "step", |
There was a problem hiding this comment.
issue: I think there's a mistake here. This code assumes that self.experiment.step method is being wrapped, but the content of self._original_methods["step"] is self.monty.step. MontyExperiment has no step method.
Since run_episode is wrapped specially, I'm a little lost as to the consequence of this mistake, if any.
def run_episode(self):
"""Run one episode until model.is_done."""
self.pre_episode()
for step, observation in enumerate(self.dataloader):
self.pre_step(step, observation)
self.model.step(observation)
self.post_step(step, observation)
if self.model.is_done or step >= self.max_steps:
break
self.post_episode(step)My question would be, are self.model.step(observation) flops included in self.total_flops? I don't think self.model.step is being wrapped.
Seeing this mistake, it might be better to organize the code into self._original_monty_methods and self._original_experiment_methods, which would then facilitate something like:
for name in self._original_monty_methods:
wrap_method(name, self.monty)
for name in self._original_experiment_methods:
wrap_method(name, self.experiment)| def unwrap(self): | ||
| """Restore original methods.""" | ||
| for method_name, original in self._original_methods.items(): | ||
| setattr(self.monty, method_name, original) |
There was a problem hiding this comment.
issue: This doesn't look correct. self._original_methods contains methods from self.experiment and self.monty. This unilaterally assigns all originals to self.monty.
Seeing this mistake, it might be better to organize the code into self._original_monty_methods and self._original_experiment_methods, which would then facilitate something like:
for method_name, original in self._original_monty_methods.items():
setattr(self.monty, method_name, original)
for method_name, original in self._original_experiment_methods.items():
setattr(self.experiment, method_name, original)
|
I think this pull request would benefit from being redone by creating a new branch from |
No description provided.