Resolution - Added keyword argument `wavelength` to `get_limits`

[marcus-oscarsson]

Added keyword argument wavelength to get_limits to be able to retrieve the limits for an arbitrary wavelength.

[marcus-oscarsson]

I'm not sure I share that idea, I think we can very well add a keyword argument in a subclass. I don't mind adding a method called get_limits_for_energy that will work just as well in this scenario.

[rhfogh]

That would depend a bit on which subclass. If you do it in ESRF-specific classes, where you control the use, I agree with you. But just in the current example there is a PX2Resolution(AbstractResolution) class with its own get_limits() function. I think it is unfortunate that you can write code what will work with HardwareObjects.Resolution, but not with AbstractResolution or PX2Resolution

[marcus-oscarsson]

Hm, but the change is to AbstractResolution so anything that inherits AbstractResolution could use this update for instance PX2Resolution or Resolution. It wont break anything, however if those sub classes override get_limit they also need to pass wavelength to be able to use this addition, is that what you mean poses a problem.

[rhfogh]

My apologies - I was getting confused about which class it was (I suppose you have been doing so much work that I was struggling to keep up ;-) ) That being so, it is indeed something one might do - much as AbstractResolution.set_limits is disallowed (overriding the superclass). The only thing remaining is a question of style. How about seeing if anyone else also thinks that this is wrong - and merging it otherwise?

[marcus-oscarsson]

Sure, no problem at all. Sorry for all the noise, I had some backlog to commit :)I don't mind otherwise adding a separate method if that is what people find more suitable.

[rhfogh]

Actually I meant that as a compliment, not a complaint. It is good that all these things are getting fixed.

[marcus-oscarsson]

:), yes I understood that. But I could also at the same time portioned out the PRs a bit better but I had this laying around and I had a good moment to make them :)

[fabcor-maxiv]

Liskov substitution principle?

[marcus-oscarsson]

I think it would be alright from a Liskov perspective, I guess right ?

[fabcor-maxiv]

I tried to look it up, and it seems like it is a violation.

But I could not find a reference that is explicit about the case of adding an optional parameter to a method of the child class.

[marcus-oscarsson]

Interesting discussion :), what principle would it violate, it does not strengthen the requirements on superclass and it does not make the requirements on the subclasses any looser.

[rhfogh]

I do not know about Liskov, but one might add: since the function does not change the internal state of the object it would also not have effects on the object history. And set_limits is (of necessity) disallowed on the subclass, which may be relevant to how much purity we can achieve.

[fabcor-maxiv]

Interesting discussion :), what principle would it violate, it does not strengthen the requirements on superclass and it does not make the requirements on the subclasses any looser.

I am not really super familiar with this topic (but it is something that is kind of in the back of my mind), so anyway I am not 100% sure about what I am about to write...

The way I understand it we have def get_limits(self) in one of the parent classes AbstractActuator:

https://github.com/mxcube/mxcubecore/blob/8f13b5ddc2ea70dfffb1dfc1a63c10e21eddb5db/mxcubecore/HardwareObjects/abstract/AbstractActuator.py#L80

And now we want to have this in the child class AbstractResolution:

https://github.com/mxcube/mxcubecore/blob/d0d74031983c894cb461440a59297e361a1ab528/mxcubecore/HardwareObjects/abstract/AbstractResolution.py#L85

And the meaning of the LSP is that we should always be able to substitute the child class AbstractResolution with the parent class AbstractActuator. But AbstractActuator.get_limits() does not know about the (optional) parameter wavelength, so we can not do a theoretical AbstractActuator.get_limits(wavelength=1.2). So maybe it is a violation? I do not know for sure. Maybe the fact that the parameter is optional make it okay from the point of view of LSP which is what I was trying to look up but could not find a clear statement for or against.

I guess it is fine. It all depends on how we make use of all this, if it breaks our code then it is bad, if our code works then it does not matter. Maybe someone will get a surprise at some point, so maybe at least adding a comment to state that we change the method signature here. Also why not add a method get_limits_for_wavelength(wavelength: float)?

[rhfogh]

I know even less than you about the underlying principles. But if you write code for the superclass (i.e. without the extra parameter) you can certainly substitute in a subclass instance - much as if you had added an extra function in the subclass. If you write code for the subclass you cannot substitute in the superclass instance, but then you are not supposed to be able to, are you?

My gut reaction was to prefer not to change the function signature at all, but apart from a small measure of confusion I do not think this will break anything.

[marcus-oscarsson]

I think respecting Liskov substitution makes things a bit complicated, I agree that its an interesting theory. However it creates quite hard restrictions on the abstractions, the API for sub classes basically can't change, it will thus only in allow for changes in behavior.

Adding a keyword argument would be a gray zone I guess, as I mentioned previously I don't mind adding a method called get_limits_for_wavelength, if that makes things more clear. That said I don't think we should strive to respect Liskov substitution.

[fabcor-maxiv]

I don't mind adding a method called get_limits_for_wavelength, if that makes things more clear

That is what I would do (I say that without knowing if it implies other changes as well or not). But if you opt for changing the signature, I will not block of course. I would not even have noticed if I had not read Rasmus' comments.

[beteva]

Interesting discussion indeed. But as all this serves to control a beamline, I guess we could be less strict and more pragmatic. We already accept that the limits of the resolution (abstract or not), depends on the wavelength and we use the current wavelength. This is hidden, though, in the get_limits method, but the wavelength is already a parameter for the underlying calculation functions. So why not give the flexibility to use any wavelength. I guess nobody will use AbstractActuator,get_limits instead of Resolution. get_limits and do not think we should add more methods, which will be more confusing than neat. This is my engineering background speaking 😄

[rhfogh]

@beteva OK by me.

[fabcor-maxiv]

@beteva am I understanding you correctly that you argue for adding the optional wavelength parameter to the original method (i.e. in AbstractActuator)? Seems like that would be fine (from a pure theoretical point of view, I have no idea in practice if it makes sense to have an optional wavelength parameter for all actuators I do not know what these classes are used for).

[beteva]

@beteva am I understanding you correctly that you argue for adding the optional wavelength parameter to the original method (i.e. in AbstractActuator)?

Actiually not. I was voting for having wavelength parameter for the resolution classes only, not for the AbstractActuator. Sorry for the confusion.

[marcus-oscarsson]

So I finally added get_limits_for_wavelength instead as adding the keyword argument seems to possibly be subject to cause some confusion.