Fix #1471: add pre-move & post-move hook places to mv-macro

[dschick]

This PR adds the pre-move & post-move hook places to mv macro.
A new sardanacustomsetting parameter named PRE_POST_MOVE_HOOK_IN_MV is introduced to disable the pre-move and post-move hook places in the mv macro by default.

[dschick]

please let me know, if you have a better idea for the name of the sardanacustomsettings parameter

[dschick]

According to the dicussion in #1472 I would simply add a

self.motors = [...]

to the mv and umv macros saving the list of involved motors in the self.motors attribute?

Do I have to put in the prepare method or is it sufficient to put in in the run method?

[reszelaz]

Do I have to put in the prepare method or is it sufficient to put in in the run method?

I would say that in the run() is enough. This attribute will be accessed in the hooks mainly. So it for the moment I would put it there.

[teresanunez]

Hi, I have tested it and it works nicely. The only point, I think we have decided to put True as default.
In the file in this PR is set to False. A part from that, i would say that it can be merged.

[reszelaz]

Thanks @teresanunez for the review!

The only point, I think we have decided to put True as default.
In the file in this PR is set to False.

See my reasoning in #1471 (comment), I already talked with @teresanunez privatelly about it and she agrees on using True as default. @dschick If you are also fine with that and there are no other opinions, could you please make the necessary change and then we could proceed with the merge.

Many thanks to all!

[dschick]

Just changed the default from False to True.
Shall I also update the docs within this PR?

[teresanunez]

Hi Daniel, thanks. Do you mean to change the comments in the file sardanacustomsettings.py ?. Yes, please change that.

[dschick]

Hi Teresa, I think I added the correct comments in the file sardanacustomsettings.py
I was more thinking of added some information to the general Sardana documentation and this new custom setting?

[teresanunez]

Hi Daniel, let´s see what Zibi says, but I think the idea is that by default the pre/post-move hooks will run with
mv/umv and they have to be explicitly disabled in the sardanacustomsettings if one does not want to have them
active.

[teresanunez]

So the default behaviour has to be True (not only the default value in the sardanacustomsettings)

[dschick]

thanks for the hint, I added the default value True to getattr

[teresanunez]

Thanks, IMO is fine now. The documentation would be great. But I think it could
be another PR, as you prefer.

[dschick]

okay, then do it in another PR and first merge this one here

[reszelaz]

Thanks to both for working on this PR!

I agree to add documentation in another PR. I can start working on that in parallel.

I have still one cooment about this one, where umv macro is not Hookable and does not announce allowsHooks hint. I've not checked it but I suspect that it won't be possible to:

1. Attach sequencer hooks hooks to it
2. Attach programmatic hooks to it
3. In the future, whenever we add more granular configuration of general hooks as described in pre- and post-move hooks not available in mv macro #1471 (comment), it won't be possible to attach general hooks only to the umv macro.

I think we would overcome all these points if wee make it inherit from Hookable, add the allowsHooks hint and then in the run() first create the mv macro and then transfer to it the hooks from the umv. What do you think? Should that work?

[teresanunez]

One should try and test, I think.

[dschick]

I have merged all of your changes as well as #1526
Finally I noticed that I have assigned only the names and not the motor objects to self.motors in umv.

I also added the self.motors to mvr and umvr.

This is maybe anther issue, but how about the self.motors in br and ubr?
Should we add there hkl or the real physical angle motors to self.motors?
I would suggest to use hkl as it is also though to get the angles in the ubr command.

Other wise everythin is working fine now for me with mv, umv, mvr, umvr.
I was not able to test the br and ubr.

[reszelaz]

Finally I noticed that I have assigned only the names and not the motor objects to self.motors in umv.

Very good, I missed that when reviewing it. Yes, the scan macros deposit the moveables objects in self.motors so I would continue doing it this way.

I also added the self.motors to mvr and umvr.

From the point of view of the pre-move or post-move hook developer, these are not strictrly necessary. At the end, what will be used are the self.motors of the mv macro object. But, it is OK to fill it, and it will be more consistent with what was
proposed in #1472 (comment).

[dschick]

From the point of view of the pre-move or post-move hook developer, these are not strictrly necessary. At the end, what will be used are the self.motors of the mv macro object. But, it is OK to fill it, and it will be more consistent with what was
proposed in #1472 (comment).

I was actually about to ask about that. If I ask for the parent = self.getParentMacro() in a pre-move hooked macro, when executing umv I do get umv returned and not mv.
In case I would get the mv macro returned as grant parrent I could access mv.motors and I would not need to add the self.motors to mvr, umvr, umv. I like this idea very much, as this would also solve the issue for ubr, br and essentially every other macro that involves the mv macro.

So is this still a bug or do I use the wrong syntax to access the corret ancestor?

[reszelaz]

Sorry about my previous message which created confusion. I was speaking without actually testing it. If it already works as you explain, then I think it is correct.

As I mentioned somewhere earlier, the fact that the umv internally executes the mv IMO an implementation detail. We must define umv (and others) as Hookable, and define the allowsHooks, and also, as you recently corrected, fill the self.motors. Otherwise, it won't be possible to attach hooks to the umv.

[dschick]

thanks for clarification, however, it would be much nicer for the future if the hooks from the internally called mv macro will be called automatically without the workaround with Hookable, allowsHooks, and self.motors.
I guess this could be solved to some extent if all move-macros inherit from the same base class, which does above mentioned logic.

Still it will be more difficult for br and ubr.

I also wrote simple uan and an macros in analogy to SPEC to move tth and th simulataneously.
In this case I always have to remember to add all the logic for the hooks and motors.
So at least inheriting would make things much eaiser for macro developer here.

[dschick]

I tested and updated br and ubr and it works as expected.
@teresanunez I decided to add the physical angles here as self.motors, instead of the hkl pseuodmotors.
Maybe we should have all the involved physical and the hkl pseudomotors listed here?
Since all of them can be changed It should not make a large difference.

[teresanunez]

I think it is fine only with the physical motors.

[reszelaz]

Thanks to both @dschick and @teresanunez for reviewing the HKL part!
I think it is then ready to be merged. @dschick can you confirm?

I decided to add the physical angles here as self.motors, instead of the hkl pseuodmotors.
Maybe we should have all the involved physical and the hkl pseudomotors listed here?

This is a very interesting point. For example, if we execute a scan or a mv on a pseudo motor, then it is the pseudo motor in the motors list. If I understand correctly the br macro is like a mv on hard coded pseudo motors (H, K and L). In this case, for symmetry, shouldn't these be the pseudo motors in the motors lists? The fact that underneath we execute a mv macro on the physical axes (angles) it is more implementation detail. I think that the pre-move and post-move hook developer would still be able to access to the physical motors from the pseudo motor object. Also, in principle, he would never get access to the mv macro object because the parent macro will be the br.

This is just my way of interpreting it, I may be wrong (I've not made any tests of that) and I rely on your decission cause you have much more experience with the HKL.

[reszelaz]

@dschick, just a side comment/question on the PRs.

I think it would be more clear to the reviewer if we avoid merging other PRs, let's call them dependency PRs, into the one of the PR, let's call it dependent PR. Whenever the dependency PRs are getting merged into the develop branch, then we could update the dependent PR with the changes from the develop branch.

This way the list of the commits will only include the ones from the dependent PR, and the list of the changes as well.

I understand the need to reflect on this kind of dependencies. So, what we could do instead, is to keep an updated list of dependency PRs in the dependent PR description (initial comment) and make comments in the dependent PR discussion whenever this list changes (so we get notifications on the updates). Or maybe there are better mechanisms/policies to solve this?

What you, and others, think about it?

[teresanunez]

Hi Zibi, the macros br and ubr do not move the pseudomotors but the real motors directly. This is why I agree with
the implemention from Daniel.

[reszelaz]

the macros br and ubr do not move the pseudomotors but the real motors directly

Yes, this is what happens inside of the macro. But from the user point of view, the parameters that you pass are the H, K and L positions, so pseudo motors' positions.

Now, let's put aside the br and analyze the mv macro example (not the mv macro executed from inside of the br), and we move a pseudo motor with the mv macro, we also pass the pseudo motor's position as a target, and in this case in the motors we store the pseudo motor and not physical motors.

[dschick]

Hi @reszelaz

@dschick, just a side comment/question on the PRs.

I think it would be more clear to the reviewer if we avoid merging other PRs, let's call them dependency PRs, into the one of the PR, let's call it dependent PR. Whenever the dependency PRs are getting merged into the develop branch, then we could update the dependent PR with the changes from the develop branch.

This way the list of the commits will only include the ones from the dependent PR, and the list of the changes as well.

I understand the need to reflect on this kind of dependencies. So, what we could do instead, is to keep an updated list of dependency PRs in the dependent PR description (initial comment) and make comments in the dependent PR discussion whenever this list changes (so we get notifications on the updates). Or maybe there are better mechanisms/policies to solve this?

What you, and others, think about it?

that is a very good idea, to keep the dependencies listed in the header. And I am sorry for the mess I produce sometimes, when fighting with git :)

I am just not so sure about, how I should use the dependencies in my PR branch without merging them from another PR branch?
What would be the correct git command for that?

Now, let's put aside the br and analyze the mv macro example (not the mv macro executed from inside of the br), and we move a pseudo motor with the mv macro, we also pass the pseudo motor's position as a target, and in this case in the motors we store the pseudo motor and not physical motors.

I actually used the physical motors just because of what @teresanunez said, as the hkl are not used as pseudomotors.
However, I agree that one might be consistent here.
BUT for the case of br the pre-move and post-move developers will not be able to access the real physical motors from the pseudo motors, so in this specific case, it is a though decision.

I think it is then ready to be merged. @dschick can you confirm?

I am happy with it. if you have tested it, it can be merged

[reszelaz]

that is a very good idea, to keep the dependencies listed in the header. And I am sorry for the mess I produce sometimes, when fighting with git :)

Don't worry! It happens to all of us:) IMHO, the best thing about Git is that it is so easy to redo things when something goes wrong.

I am just not so sure about, how I should use the dependencies in my PR branch without merging them from another PR branch?
What would be the correct git command for that?

I'm not an expert here, but I would keep two branches: the one for the PR needs. And another one where I would put it together with the dependencies just for doing tests. There may be other ways, like for example isaacs/github#959 (comment), but I think it may need that all the branches are in the upstream repo and not in the forks. Anyway, from the Sardana point of view, it is not worth to explore it since we will be moving to GitLab soon.

BUT for the case of br the pre-move and post-move developers will not be able to access the real physical motors from the pseudo motors, so in this specific case, it is a though decision

Not directly, but the following should work :

@macro()
def pre_move_hook(self):
    motors = []
    for moveable in self.parent_macro.motors:
        if moveable.type == "PseudoMotor":
            motors.extend([self.getMotor(name) for name in moveable.physical_elements])
        else:
            motors.append(moveable)

You could also use moveable.elements instead of moveable.physical_elements if you are sure that there is only one pseudo layer.

Again, I'm happy with your decission. I just wanted to share with you this observations. Thanks again @teresanunez and @dschick !

[teresanunez]

Hi Zibi, the point is that the macro br does not use the pseudomotors at all ... As Daniel and myself said, the moveables
used are the motors of the angles, but not the motors corresponding to the coordinates h, k, l. So it is not that we move
the physical motors through the pseudomotors (like all the pseudomotors do) but we move directly the motors, the
pseudomotors are not used in the macro at all.

[dschick]

in addition what @teresanunez said, you cannot move only L by umv L as it is not a typical pseudomotor, as HKL dependent many physical motors and the dependency between hkl and physical motors are not one-to-one.

[reszelaz]

@teresanunez many thanks again for the clarification. I see the point about the implementation. If possible, I still would like to ask you some questions about it (just to understand better how the diffractometers are being used by the users), but since it is not urgent let's postpone it to the follow-up meeting. Could you please remind me about it if I forget to ask the next time we meet:)

I'm merging, thanks to all involved in this work!

[teresanunez]

Hi Zibi, yes, no problem (I hope I remember that ... one of us should do).

[cpascual]

Just a note: gitlab already has a specific field for dependencies in MRs. I am just mentioning it in case you prefer waiting for migration instead of deciding on an ad hoc solution On 29 March 2021 13:06:05 CEST, reszelaz ***@***.***> wrote: ***@***.***, just a side comment/question on the PRs.

…

I think it would be more clear to the reviewer if we avoid merging other PRs, let's call them dependency PRs, into the one of the PR, let's call it dependent PR. Whenever the dependency PRs are getting merged into the develop branch, then we could update the dependent PR with the changes from the develop branch. This way the list of the commits will only include the ones from the dependent PR, and the list of the changes as well. I understand the need to reflect on this kind of dependencies. So, what we could do instead, is to keep an updated list of dependency PRs in the dependent PR description (initial comment) and make comments in the dependent PR discussion whenever this list changes (so we get notifications on the updates). Or maybe there are better mechanisms/policies to solve this? What you, and others, think about it? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #1480 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[reszelaz]

Just a note: gitlab already has a specific field for dependencies in MRs. I am just mentioning it in case you prefer waiting for migration instead of deciding on an ad hoc solution

Very good news! Thanks for letting us know. Yes, it is better to wait for the GitLab.