feat: refactor source class #690
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
========================================
Coverage 88.69% 88.70%
========================================
Files 39 39
Lines 5653 5994 +341
========================================
+ Hits 5014 5317 +303
- Misses 639 677 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
get the data from the default parameters
There was a problem hiding this comment.
get test results from parameters classe
There was a problem hiding this comment.
Only rayfile, luminaire default parameters implemented. No parameters implemented for for sensor class.
There was a problem hiding this comment.
get default values from defaults class
| def __init__( | ||
| self, | ||
| flux: source_pb2, | ||
| default_values: Union[bool, SourceRayfileParameters, SourceLuminaireParameters], |
There was a problem hiding this comment.
bool is needed.
case 1: DefaultParameters instance provided -> create based on values provided from parameter instance (it can be default instance or user-defined instance)
case 2: None or True -> use the default parameter instance from constants.py class.
case 3: False -> use the back-end property when loading an existing object
| metadata: Optional[Mapping[str, str]] = None, | ||
| source_instance: Optional[ProtoScene.SourceInstance] = None, | ||
| default_values: bool = True, | ||
| default_values: Optional[bool, SourceLuminaireParameters] = None, |
| # Default values | ||
| self.set_flux_from_intensity_file().set_spectrum().set_incandescent() | ||
| self.set_axis_system() | ||
| if default_values is not False: |
| def __init__( | ||
| self, | ||
| rayfile_props: scene_pb2.RayFileProperties, | ||
| default_values: Optional[bool, SourceRayfileParameters], |
There was a problem hiding this comment.
why not use None and source ray file what happens,
maybe rename: default parameters?
if None Default will be taken expept if source is created from file
valid for all inits
| if default_values is not False: # True, SourceRayfileParameters as default | ||
| if not isinstance(default_values, SourceRayfileParameters): # True as default | ||
| default_values = SourceRayfileParameters() | ||
| # Default values | ||
| self.set_spectrum_from_ray_file() | ||
| self.ray_file_uri = default_values.ray_file_uri | ||
| match default_values.flux_type: | ||
| case FluxType.LUMINOUS: | ||
| self.set_flux().set_luminous() | ||
| self.set_flux().value = default_values.flux_value | ||
| case FluxType.RADIANT: | ||
| self.set_flux().set_radiant() | ||
| self.set_flux().value = default_values.flux_value | ||
| case FluxType.FROM_FILE: | ||
| self.set_flux_from_ray_file() | ||
| case _: | ||
| raise ValueError( | ||
| f"Unsupported flux type: {type(default_values.flux_type).__name__}" | ||
| ) | ||
| self.axis_system = default_values.axis_system | ||
| self.set_exit_geometries().geometries = default_values.exit_geometry |
There was a problem hiding this comment.
Check for instances to just catch the relevant part and the if None but no instance create parameters
SMoraisAnsys
left a comment
SMoraisAnsys
left a comment
There was a problem hiding this comment.
Not sure about the current status of the PR but do you want to proceed with classes containing a single attribute (I would encourage to avoid that if possible) ? See classes SOURCE, LUMINOUS, ...
| # Validation: restrict flux_type | ||
| if self.flux_type not in {FluxType.FROM_FILE, FluxType.LUMINOUS, FluxType.RADIANT}: | ||
| raise ValueError( | ||
| f"Invalid flux_type '{self.flux_type}'. Must be FROM_FILE, LUMINOUS, or RADIANT." |
There was a problem hiding this comment.
Identitical validation is there in both classes, i would suggest to create a base class (dataclass) and add this post__init__ and also validate_flux_type. Also you can centralize validation logic in enum class itself. (as a property)
There was a problem hiding this comment.
elaborating the suggestion:
Since the __post_init__() method in both dataclasses mainly performs type checking and validation, it would be better to introduce a BaseSourceParameters class that centralizes all common attributes and logic.
This base class can handle:
- Initialization of shared attributes (flux_type, flux_value, axis_system)
- Common validation through helper methods (
validate_flux_type(), validate_axis_system()) - A placeholder
specific_validation()method that can be overridden by subclasses for class-specific logic.
@dataclass
class BaseSourceParameters:
flux_type: FluxType = FluxType.FROM_FILE
flux_value: Optional[float] = None
axis_system: list[float] = field(default_factory=lambda: ORIGIN.copy())
def __post_init__(self):
self.validate_flux_type()
self.validate_axis_system()
self.specific_validation() # Hook for subclass-specific checks
def validate_flux_type(self):
# Common validation logic
...
def validate_axis_system(self):
# Common axis system validation
...
def specific_validation(self):
# To be implemented by subclasses
pass
- For the FluxType enum, it would also be a good idea to add a @classmethod that returns the valid enum members for source parameters, instead of hardcoding the allowed set in multiple places.
@classmethod
def get_allowed_for_source_parameters(cls) -> set["FluxType"]:
return {cls.FROM_FILE, cls.LUMINOUS, cls.RADIANT}
allowed_types = FluxType.get_allowed_for_source_parameters()
print(allowed_types)
# Output: {<FluxType.FROM_FILE: 'from_file'>, <FluxType.LUMINOUS: 'luminous'>, <FluxType.RADIANT: 'radiant'>}
```
There was a problem hiding this comment.
Another comment will be changing this source constants, to another file, to make the code neat
Description
need some testing and proper definition
Add properties to SourceLuminaire:
add properties to SourceRayfile:
add properties to SourceSurface
Issue linked
Refactor source #684
Checklist
feat: add optical property)