Missing inheriting arguments of observer base class

[munechika-koyo]

Hello,

I noticed that some imaging observer classes could not take arguments defined in base classes like Observer2D.
I mean:

PinholeCamera(
    (512, 512),
   min_wavelength=375  # <-- TypeError: __init__() got an unexpected keyword argument 'min_wavelength'
)

Therefore, How do you think of adding **kwargs parameters into such a imaging object's constructor arguments like:

cdef class PinholeCamera(Observer2D):
    def __init__(self, pixels, fov=None, sensitivity=None, frame_sampler=None, pipelines=None, parent=None, transform=None, name=None, **kwargs):

Thank you in advance.

[jacklovell]

Wouldn't this mean these observers would silently ignore any options they don't support? Sounds like that would be more likely to lead to unexpected behaviour. In the example above, an end user might reasonably expect that if the observer accepts min_wavelength=375 it would not consider any emission at shorter wavelengths, but that behaviour depends entirely on the pipelines attached to the observer.

Personally I would rather objects raise an error if they are supplied with options they don't support, as it's then obvious to the user that their calling code is wrong and they should fix it. Far too often I see "the code runs without errors, therefore the answer must be correct" and if this can be avoided by erroring instead that's fine by me.

Happy to hear from others though.

[vsnever]

Wouldn't this mean these observers would silently ignore any options they don't support?

If **kwargs parameters are only added to inherited classes, the base class constructor will throw an error if an unsupported parameter is passed. So, this is a safe approach.

class ClassA():

    def __init__(self, var1='a', var2='b'):
        self.var1 = var1
        self.var2 = var2


class ClassB(ClassA):

    def __init__(self, var1='a', **kwargs):

        super().__init__(var1=var1, **kwargs)


>>> b = ClassB(var1='a', var2='b', var3='c')
>>> TypeError: ClassA.__init__() got an unexpected keyword argument 'var3'

[Mateasek]

Before there is a decision made, I have a question. What is the reason the parameters were not included in the init definition? Is it an honest mistake or is there a reason?

Then I would suggest against using args/kwargs in such basic classes as observers. The reason is that in my opinion it makes the code more messy and much more difficult to debug. Implementing kwargs would I think decrease Raysect's code clarity. Every beginner should be able to write a factory function which accepts the kwargs argument, handles individual use cases and returns an instance of the observer.

If there is the decision to add the missing arguments, I would add them as optional.

[vsnever]

All non-imaging observers accept arguments of the base observer classes explicitly, for example FibreOptic:

source/raysect/optical/observer/nonimaging/fibreoptic.pyx

Lines 75 to 87 in 66fdf70

	def __init__(self, pipelines=None, acceptance_angle=None, radius=None, parent=None, transform=None, name=None,
	render_engine=None, pixel_samples=None, samples_per_task=None, spectral_rays=None, spectral_bins=None,
	min_wavelength=None, max_wavelength=None, ray_extinction_prob=None, ray_extinction_min_depth=None,
	ray_max_depth=None, ray_importance_sampling=None, ray_important_path_weight=None, quiet=False):
	pipelines = pipelines or [SpectralPowerPipeline0D()]
	super().__init__(pipelines, parent=parent, transform=transform, name=name, render_engine=render_engine,
	pixel_samples=pixel_samples, samples_per_task=samples_per_task, spectral_rays=spectral_rays,
	spectral_bins=spectral_bins, min_wavelength=min_wavelength, max_wavelength=max_wavelength,
	ray_extinction_prob=ray_extinction_prob, ray_extinction_min_depth=ray_extinction_min_depth,
	ray_max_depth=ray_max_depth, ray_importance_sampling=ray_importance_sampling,
	ray_important_path_weight=ray_important_path_weight, quiet=quiet)

Both imagine and non-imaging observers should accept base class arguments in the same way. There are currently only six imaging observers in Raysect, I don't see a problem adding base class arguments to their initialisers explicitly. Indeed, it's more descriptive because it reduces the chance that the user will forget to pass an argument.

[vsnever]

There are currently only six imaging observers in Raysect, I don't see a problem adding base class arguments to their initialisers explicitly. Indeed, it's more descriptive because it reduces the chance that the user will forget to pass an argument.

The drawback of this is that if a new argument is added to the base class, all inherited classes will also have to be updated.

[CnlPepper]

Its a deliberate choice. The imaging observer constructors focus on the configuration of the "camera" geometry/processing. The ray generation configuration is performed by modifying default values via attributes. The additional parameters could be added, but it would make the calling signatures messier than the 0D observers (which really should have the same interface as the imaging observers.

[CnlPepper]

Thinning down the messy non-imaging constructors would be my preferred choice. Not all configuration needs to be via constructor.

[munechika-koyo]

Thank you for all your comments:)
If such a definition is intended, we should fix documentation (docstrings) explaining imaging observers because they can confuse users.
Here the PinholeCamera description says that **kwargs from Observer2D and _ObserverBase can be specified as the constructor's parameters.

[munechika-koyo]

In the example above, an end user might reasonably expect that if the observer accepts min_wavelength=375 it would not consider any emission at shorter wavelengths, but that behaviour depends entirely on the pipelines attached to the observer.

@jacklovell
I do not really understand however, I think the range of wavelength considered in the ray tracing should be decided by such a parameter not by pipelines.
They seem to be in charge of post-processes to finalize ray-traced spectrums like integrating or weighting them for each pixel.
So, I think the observer with a specific min_wavelength=375 does not care about the shorter region <375. Is it correct?