Fail the measurement when InstrJob timeout #62

lcontami · 2019-12-05T10:33:19Z

I did not stop the measurement stopping in the InstrJob for two reasons;

for now InstrJob does not have access to the root task
one could want to take specific action when the InstrJob timeouts; these could be added in the cancel() method of the InstrJob but cancel() is also called when the measurement is stopped and one could want different behaviors (besides, again cancel does not have access to the root task)

In the end it seemed more logical to me to stop the measurement in the Task when the job fails rather than in the Job itself, but feel free to change if you want.

MatthieuDartiailh

The failing test should be not too bad to fix. Let me know if you run into trouble with it.

MatthieuDartiailh · 2019-12-05T14:10:53Z

exopy_hqc_legacy/instruments/drivers/visa/cryomagnetics_cs4.py

    def stop_sweep(self):
-        """Stop the field sweep at the current value.
+        """Stop the field sweep at the current value, and turn of the switch heater.


Why this change in behavior ?

It was specified in the job after job.cancel() was called (which in turn calls this method), it is more logical to integrate it directly to the method called as cancel()

I disagree, you may stop a sweep but need to run a new one right after in which case automatically closing the switch heater would be a bad idea (this is hypothetical). Also if you wanted to go that way you need to be sure all driver are consistent.

MatthieuDartiailh · 2019-12-05T14:12:00Z

exopy_hqc_legacy/tasks/tasks/instr/apply_mag_field_task.py

            # set the magnetic field
            job = driver.sweep_to_field(target_value, self.rate)
            normal_end = job.wait_for_completion(self.check_for_interruption,
                                                 timeout=60,
                                                 refresh_time=10)
+            print('field:', driver.read_persistent_field())


If we want to keep this it should be a logger call.

No this was for debugging purpose I'll remove it

MatthieuDartiailh · 2019-12-05T14:13:32Z

exopy_hqc_legacy/tasks/tasks/instr/apply_mag_field_task.py

-            job.cancel()
-            driver.heater_state = 'Off'
-            sleep(self.post_switch_wait)
+            job.cancel() # stops the sweep and turn off the switch heater


I don't see why cancelling the job should always close the switch heater. Also you only modified the cg4 and cs4 but I believe the Oxford source behaves differently.

I didn't add this but it seems rather reasonable to me since the switch heater increases the He consumption

The oxford source is broken so I have not updated the driver (since I am not able to test it and I didn't want to leave something buggy behind)

I am fine with closing the heater my concern is the same above regarding the location of that code inside cancel.

MatthieuDartiailh · 2019-12-05T14:14:58Z

exopy_hqc_legacy/tasks/tasks/instr/apply_mag_field_task.py

-            return False
+            # if not normal_end, fail the measurement
+            self.root.should_stop.set()
+            raise ValueError(cleandoc('''Field source did not set the field to 


This is debatable since it will appear as a hard failure even if the measurement was properly interrupted. This point to the need of differentiating between stop and timeout as I was suggesting in #61 .

Mmm I was under the impression that when the measurement was manually stopped, the program stopped before going there but it is not clear to me how the stop signal is dealt with.
I am fine with #61 !

lcontami · 2019-12-05T14:39:56Z

I tried to integrate your TimeOut
again, not very clear to me what stops the measurement when a stop signal is manually sent but what I was aiming at is:

if timeout, then stop the sweep and turn off sw heater, return a ValueError
if the measure is manually stopped, stop and do nothing more

lcontami · 2019-12-05T14:40:14Z

Did not look at Travis error yet...

MatthieuDartiailh · 2019-12-05T14:49:16Z

Why don't you want to close the switch heater when the user interrupt the scan ?

Once again my argument about cancel is not about whether or not to close the switch in the context of this particular case, but that more generally stopping a field sweep does not necessarily imply that the switch heater should be closed.

At the beginning of each task (if the feature has not been disabled) the should_stop event is checked and if set the execution abort, this quickly lead to stopping the measurement.

lcontami · 2019-12-06T11:01:41Z

If the idea of closing the sw heater is to save He when the measurement stop by itself overnight, this is not necessary if I am in front of the computer and I am stopping the measurement.
Besides, turning off the sw heater takes some time which is annoying

lcontami · 2019-12-06T12:34:00Z

Honestly Madar I am lost between all these comments, I don't understand what is bothering you but I really don't have a strong opinion on how the code is organised.
I just have an opinion on how the measurement should behave so if you want to or want me to move some bits around tell me as long as the behavior stays the same.

MatthieuDartiailh · 2019-12-06T16:13:05Z

tests/tasks/tasks/instr/test_apply_mag_field_task.py

@@ -37,7 +37,7 @@ def setup(self):
                                      parallel={'activated': False})
        self.root.add_child_task(0, self.task)

-        self.root.run_time[DRIVERS] = {'Test': (InstrHelper,
+        self.root.run_time[DRIVERS] = {'Test': (InstrHelper({'heater_state': ['On', 'Off']}),


This dict entry should be moved to the settings.

MatthieuDartiailh · 2019-12-06T16:13:16Z

I get your argument for not closing the switch heater after a manual stop. Currently there is one issue I believe: when stopping the measurement the job will return false but since it it not cancelled the magnet will keep ramping which seems surprising.

To fix that I believe no matter the reason we stop waiting for you should cancel the job. However cancelling should leave the switch heater alone and you could deal with the switch only in the case of a timeout.

For the test I left a comment.

lcontami · 2019-12-09T13:32:13Z

Thanks, I had not realized I had indeed changed the way a measurement stop was handled, in a bad manner. Tell me if what I have proposed suits you, I have edited mainly the InstrJob but I can also put these edits in the task if you think it best. Correct me if I'm wrong but when the measurement has been stopped manually I don't need to write self.root.should_stop.set() since it is already.

Unfortunately the fridge is not cold anymore so I won't be able to test this code...

MatthieuDartiailh

This looks nice. I would need to think a bit more about some things if this was included elsewhere but for this package it is fine.

MatthieuDartiailh · 2019-12-09T16:28:15Z

exopy_hqc_legacy/instruments/drivers/visa/cryomagnetics_g4.py

@@ -62,7 +63,6 @@ def target_field(self):
        """Field that the source will try to reach.

        """
-        # in T
        return float(self.ask('ULIM?').strip(' kG')) / 10


If you keep the division the field is not given in kG.

lcontami · 2019-12-17T12:56:06Z

This does not work anymore... When the measurement is manually stopped the sweep stops (activity = 'Hold') but the measurement stays in "stopping" and does not stop.

MatthieuDartiailh · 2019-12-18T13:44:23Z

Do you have a log, config file or something similar I could look at ?

Fail the measurement when InstrJob timeout

ec0e859

MatthieuDartiailh mentioned this pull request Dec 5, 2019

Stop the measurement if setting the field timeout #61

Closed

MatthieuDartiailh reviewed Dec 5, 2019

View reviewed changes

Integrating the code of Madar

585eac0

MatthieuDartiailh reviewed Dec 6, 2019

View reviewed changes

lcontami added 3 commits December 9, 2019 10:43

Stopping the sweep when the measure is stopped !

755e0d0

correct the test ?

d1abdb4

for the test again

177f08a

mistakes

abe5ed2

MatthieuDartiailh approved these changes Dec 9, 2019

View reviewed changes

lcontami and others added 3 commits December 9, 2019 15:28

fix test

ce54d25

test again

5c75205

small bug in c4g

2c1c96a

MatthieuDartiailh reviewed Dec 9, 2019

View reviewed changes

lcontami and others added 4 commits December 9, 2019 17:30

...

b53565f

...

f53a5b6

small typo

55d670f

small mistake

b032f3d

Base automatically changed from master to main January 19, 2021 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail the measurement when InstrJob timeout #62

Fail the measurement when InstrJob timeout #62

lcontami commented Dec 5, 2019

MatthieuDartiailh left a comment

MatthieuDartiailh Dec 5, 2019

lcontami Dec 5, 2019

MatthieuDartiailh Dec 5, 2019 •

edited

Loading

MatthieuDartiailh Dec 5, 2019

lcontami Dec 5, 2019

MatthieuDartiailh Dec 5, 2019

lcontami Dec 5, 2019

MatthieuDartiailh Dec 5, 2019

MatthieuDartiailh Dec 5, 2019

lcontami Dec 5, 2019

lcontami commented Dec 5, 2019

lcontami commented Dec 5, 2019

MatthieuDartiailh commented Dec 5, 2019

lcontami commented Dec 6, 2019

lcontami commented Dec 6, 2019

MatthieuDartiailh Dec 6, 2019

MatthieuDartiailh commented Dec 6, 2019

lcontami commented Dec 9, 2019 •

edited

Loading

MatthieuDartiailh left a comment

MatthieuDartiailh Dec 9, 2019

lcontami commented Dec 17, 2019

MatthieuDartiailh commented Dec 18, 2019

Fail the measurement when InstrJob timeout #62

Are you sure you want to change the base?

Fail the measurement when InstrJob timeout #62

Conversation

lcontami commented Dec 5, 2019

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthieuDartiailh Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcontami commented Dec 5, 2019

lcontami commented Dec 5, 2019

MatthieuDartiailh commented Dec 5, 2019

lcontami commented Dec 6, 2019

lcontami commented Dec 6, 2019

Choose a reason for hiding this comment

MatthieuDartiailh commented Dec 6, 2019

lcontami commented Dec 9, 2019 • edited Loading

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcontami commented Dec 17, 2019

MatthieuDartiailh commented Dec 18, 2019

MatthieuDartiailh Dec 5, 2019 •

edited

Loading

lcontami commented Dec 9, 2019 •

edited

Loading