Refactor and consolidate generator set functionality in the domain layer #866
Conversation
frodehk
force-pushed
the
refactor/move-generator-set-dto-core-models
branch
from
aaac32e to
2127830
Compare
frodehk
changed the title
src/libecalc/domain/infrastructure/energy_components/generator_set/generator_set_component.py
Outdated
Show resolved
Hide resolved
src/libecalc/domain/infrastructure/energy_components/generator_set/generator_set_component.py
Outdated
Show resolved
Hide resolved
src/libecalc/domain/process/generator_set/generator_set_process_unit.py
Outdated
Show resolved
Hide resolved
| def __eq__(self, other): | ||
| if not isinstance(other, GeneratorSetProcessUnit): | ||
| return False | ||
| return ( | ||
| self.typ == other.typ | ||
| and self.headers == other.headers | ||
| and self.data == other.data | ||
| and self.get_name() == other.get_name() | ||
| and self.get_id() == other.get_id() | ||
| and self.energy_usage_adjustment_constant == other.energy_usage_adjustment_constant | ||
| and self.energy_usage_adjustment_factor == other.energy_usage_adjustment_factor | ||
| ) |
There was a problem hiding this comment.
The __eq__ method is being used indirectly in the test TestGeneratorSet.test_valid. It compares two GeneratorSetProcessUnit objects within a dictionary using assert. When the __eq__ method is commented out, Python defaults to comparing object references, which causes the test to fail because the two objects are different instances, even though their attributes are identical.
But it is only one test failing, so it is not widely used! If we remove it, we should rewrite the test to compare the attributes directly I guess (instead of relying on equality).
src/libecalc/domain/infrastructure/energy_components/generator_set/generator_set_component.py
Show resolved
Hide resolved
src/libecalc/domain/infrastructure/energy_components/generator_set/generator_set_component.py
Show resolved
Hide resolved
| model_data.setdefault("name", "generator_set_sampled_default_name") | ||
| return GeneratorSetProcessUnit(**model_data) |
There was a problem hiding this comment.
This is what I missed, so ignore the other comment about something missing.
I think we should be able to improve this mapping a bit, instead of using _default_facility_to_dto_model_data and EnergyModelFactory we could add a mapper for GeneratorSetProcessUnit explicitly. Instead of reading headers and data out of a resource we could pass resource in. That would require us to move the resource interface into domain. This is where I think we should have two domains, one for the full context of all periods, and one for the core of ecalc (single timestep process simulation). A resource makes sense in one, but not the other IMO.
Further down the line we could move the mapping into ReferenceService. That gives us the option to validate References 'on-demand' instead of instantly.
There was a problem hiding this comment.
almost felt like I wrote a part of this myself :p tbc
There was a problem hiding this comment.
A new mapper for the GeneratorSetProcessUnit is created, and Resource is moved to domain (requires update of import in api/api/domain/resource.py). Should we continue to identify the boundaries between the two suggested domains (full context vs single time step)? And then move relevant classes and logic into their respective domains? I guess we should do this separately from this PR, as we will get a lot of changes?
Should we move the mapping into ReferenceService right away, or is it better to wait until the domain boundaries are more clear?
Why is this pull request needed?
This refactoring introduces a more structured and modular approach to the generator set functionality by leveraging the
GeneratorSetEnergyComponent(extendingEnergyComponent) in domain.infrastructure.generator_set andGeneratorSetProcessUnit(extendingProcessUnit) in domain.process.generator_set.What does this pull request change?
Genset(deleted from infrastructure.generator_set) has been moved toGeneratorSetEnergyComponentandGeneratorSetProcessUnitGeneratorSetProcessUnitin domain.process.generator_set.DTOs (for generator sets) are removed because their roles have become ambiguous and redundant over time. The responsibilities previously handled by the DTO layer have been distributed more appropriately:
Direct input handling: Data that was previously passed through DTOs is now provided directly to the relevant classes, improving clarity and reducing unnecessary abstraction.
Role reassignment: The
GeneratorSetEnergyComponentandGeneratorSetProcessUnitclasses have taken over the responsibilities of the DTOs. These classes now encapsulate the logic and data handling, ensuring a more cohesive and modular design.This refactoring eliminates ambiguity, improves separation of concerns, and aligns the code with a cleaner architecture.