[Econ] Refactor Preprocess - #3197
Conversation
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
Initial review. Waiting for the PR writeup to fully evaluate but initial thoughts are similar to other economics feedback where I think more specificity in naming is really important given how complex the code is and how much mapping is referencing is going on. Happy to re-review again when ready.
| return (self.section, self.name) | ||
|
|
||
| @abstractmethod | ||
| def process(self) -> dict[str, Any]: |
There was a problem hiding this comment.
is this running the preprocess process or just setting the context for the preprocessing to happen? Regardless, maybe it could be more specific as well - setup_preprocessing_entry() for example
| self.im = InputManager() | ||
| self.om = OutputManager() | ||
| self.available_input_keys: Set[str] = self._load_available_input_keys() | ||
| self.context = PreprocessingContext(self.im, self.om) |
There was a problem hiding this comment.
what's the advantage of passing these as args vs just instantiating them in the PreprocessingContext class? I ask mainly because we don't really do this elsewhere in the code.
There was a problem hiding this comment.
I will change it to instantiating them in the PreprocessingContext class as the rest of the codebase
| self.context = context | ||
|
|
||
| @property | ||
| def key(self) -> tuple[str, str]: |
There was a problem hiding this comment.
More specific name here - maybe economic_map_key or something to distinguish what it represents.
| """Shared services for economics preprocessing. | ||
|
|
||
| Houses :class:`PreprocessingContext`, a small facade over the | ||
| :class:`~RUFAS.input_manager.InputManager` and | ||
| :class:`~RUFAS.output_manager.OutputManager` that exposes the data-access, | ||
| pricing, scenario, and aggregation helpers shared by the main | ||
| :class:`~RUFAS.EEE.economics.preprocessing.EconomicPreprocessor` and by the | ||
| special-case handlers in :mod:`RUFAS.EEE.economics.special_cases`. | ||
|
|
||
| Keeping these helpers in one place lets the main preprocessor and every | ||
| special-case handler resolve InputManager data, fall back to default prices, | ||
| enumerate scenarios, and aggregate value series through a single, tested | ||
| implementation. | ||
| """ |
There was a problem hiding this comment.
Do you envision these top-level comments being part of what gets merged in eventually?
matthew7838
left a comment
matthew7838
left a comment
There was a problem hiding this comment.
Commenting with the same verdict as Niko. Also, I have a few naming suggestions:
| now | to |
|---|---|
special_cases/ |
handler/ |
special_cases/base.py |
handler/handler.py |
SpecialCaseHandler |
LineItemHandler |
preprocessing_context.py |
data_processor.py |
PreprocessingContext |
EconomicDataProcessor |
| # from an input file; used only in InputManager validation messages. | ||
| COMPUTED_PREPROCESSING_INPUT_PATH = Path("<computed: EconomicPreprocessor.preprocess>") | ||
|
|
||
| SPECIAL_CASE_HANDLERS: list[type[SpecialCaseHandler]] = [ |
There was a problem hiding this comment.
I think this would be better if moved to the special case init file, and then we don't have to import every special case class in this file.
| handlers = [handler_cls(self.context) for handler_cls in SPECIAL_CASE_HANDLERS] | ||
| return {handler.key: handler for handler in handlers} |
There was a problem hiding this comment.
Should add a warning for mismatch
| def aggregate(self, values: list[float], desc: str) -> float | None: | ||
| """Aggregate values according to a textual description.""" | ||
| if not values: | ||
| return None | ||
| d = desc.lower() if isinstance(desc, str) else "" | ||
| if "average" in d or "mean" in d: | ||
| return Aggregator.average(values) | ||
| if "product" in d: | ||
| return Aggregator.product(values) | ||
| if "divide" in d or "ratio" in d: | ||
| result = Aggregator.division(values) | ||
| if result is not None: | ||
| return result | ||
| if "subtract" in d or "difference" in d: | ||
| result = Aggregator.subtraction(values) | ||
| if result is not None: | ||
| return result | ||
| if "standard deviation" in d or "std" in d: | ||
| return Aggregator.standard_deviation(values) | ||
| # Default aggregation is sum | ||
| return Aggregator.sum(values) |
There was a problem hiding this comment.
This could potentially be moved to util.py
b8d312f to
e113467
Compare
3 participants
Context
Issue(s) closed by this pull request: closes #
What
Why
How
Test plan
Input Changes
Output Changes
Filter