P11 develop #800

agruzinov · 2023-10-05T13:51:52Z

Here is another round of debug after upgrading to the latest code (mostly in DESY HO).

…y for shutter-like objects inside P11 HO (Bixente)

fabcor-maxiv · 2023-10-05T14:05:09Z

mxcubecore/HardwareObjects/MotorsNPosition.py

@@ -121,7 +121,7 @@ def load_positions(self):
    def get_position_list(self):
        return list(self._positions.keys())

-    def get_properties_(self, position_index, property_name):
+    def get_properties(self, position_index, property_name):


If I am not mistaken, there is already a get_properties in one of the parent classes. Namely in HardwareObjectNode, via HardwareObject and AbstractActuator.

Thanks for the comment. Changed.

beteva · 2023-10-07T06:53:21Z

mxcubecore/HardwareObjects/DESY/P11DetectorCover.py

    @unique
    class BaseValueEnum(Enum):
-     """Defines only the compulsory values."""
-     OPEN = "OPEN"
-     CLOSED = "CLOSED"
-     MOVING = "MOVING"
-     UNKNOWN = "UNKNOWN"
-
-    VALUES=BaseValueEnum
+        """Defines only the compulsory values."""
+
+        OPEN = "OPEN"
+        CLOSED = "CLOSED"
+        MOVING = "MOVING"
+        UNKNOWN = "UNKNOWN"
+
+    VALUES = BaseValueEnum


As it inherits from AbstractShutter, no need to do this. Please see my comment comment below

beteva · 2023-10-07T07:04:41Z

mxcubecore/HardwareObjects/DESY/P11FastShutter.py

    def get_value(self):
        return self.update_fast_shutter()

+    @property
    def is_open(self):
        return self.get_value() == self.VALUES.OPEN

+    @property
    def is_closed(self):
        return self.get_value() == self.VALUES.CLOSED



No need to define FastShutterVakue enum. As it inherits from AbstractNState, uou can simply do it in your xml file, like this:
<values>{"OPEN": "Open", "CLOSED": "Closed"}</values>

Thanks for the comment. What is best way to adress it from the code?

Can you, please, be more explicit. The above comment means, that your self.VALUES.OPEN.value is "Open", as you probably need.

Here is my xml:
FastShutter
<tangoname>p11/shutter/fs</tangoname> <channel type="tango" name="value" polling="500">value</channel> <values>{"OPEN": "Open", "CLOSED": "Closed"}</values>

And mxcube is not happy:

File "/mxcubecore/mxcubecore/HardwareRepository.py", line 545, in parse_xml
hardware_obj = HardwareObjectFileParser.parse_string(xml_string, ho_name)
File "/mxcubecore/HardwareObjectFileParser.py", line 78, in parse_string
xml.sax.parseString(str.encode(xml_hardware_object), cur_handler)
File "/usr/lib/python3.7/xml/sax/init.py", line 48, in parseString
parser.parse(inpsrc)
File "/usr/lib/python3.7/xml/sax/expatreader.py", line 111, in parse
xmlreader.IncrementalParser.parse(self, source)
File "/usr/lib/python3.7/xml/sax/xmlreader.py", line 125, in parse
self.feed(buffer)
File "/usr/lib/python3.7/xml/sax/expatreader.py", line 217, in feed
self._parser.Parse(data, isFinal)
File "../Modules/pyexpat.c", line 417, in StartElement
File "/usr/lib/python3.7/xml/sax/expatreader.py", line 333, in start_element
self._cont_handler.startElement(name, AttributesImpl(attrs))
File "HardwareObjectFileParser.py", line 276, in startElement
new_object = instanciate_class(module_name, class_name, object_name)
HardwareObjectFileParser.py", line 105, in instanciate_class
module = load_module(module_name)
File "/gpfs/local/shared/MXCuBE/mxcubecore/mxcubecore/HardwareObjectFileParser.py", line 91, in load_module
return import(hardware_object_name, globals(), locals(), [""])
File "mxcubecore/HardwareObjects/DESY/P11FastShutter.py", line 41, in
class P11FastShutter(AbstractNState):
File "/usr/lib/python3.7/enum.py", line 862, in unique
for name, member in enumeration.members.items():
AttributeError: type object 'P11FastShutter' has no attribute 'members'
2023-11-08 18:23:30,869 |HWR |ERROR | Failed to load Hardware object /fast-shutter

beteva · 2023-10-07T07:06:55Z

mxcubecore/HardwareObjects/DESY/P11Shutter.py

    @unique
    class BaseValueEnum(Enum):
-     """Defines only the compulsory values."""
-     OPEN = "OPEN"
-     CLOSED = "CLOSED"
-     MOVING = "MOVING"
-     UNKNOWN = "UNKNOWN"
-
-    VALUES=BaseValueEnum
+        """Defines only the compulsory values."""
+
+        OPEN = "OPEN"
+        CLOSED = "CLOSED"
+        MOVING = "MOVING"
+        UNKNOWN = "UNKNOWN"
+
+    VALUES = BaseValueEnum


As you inherit from AbstractShutter, no need to do this. Please see my comment below.

beteva · 2023-10-07T07:17:37Z

Dear @agruzinov,
there is a way to add extra values, without redefining the BaseValueEnum. The values are always accessible via self.VALUES, as you can find it attributed in this case in AbstractShutter. If you want to add extra values to this enum, you can add it like this:

def _initialise_values(self):
        """Add additional, known in advance states to VALUES"""
        values_dict = {item.name: item.value for item in self.VALUES}
        values_dict.update(
            {
                "MOVING": "In Motion",
                "UNUSABLE": "Temporarily not controlled",
            }
        )
        self.VALUES = Enum("ValueEnum", values_dict)

And in the init, you simply add:
self._initialise_values()
Thus you simply add whatever you need. Please have a look in the ShutterMockup

fabcor-maxiv

_get_property vs. get_property, see earlier inline comment

…ate changes in qt part-multi_state_brick.py (PR800)

…n TangoMotor.py

…s in the beamline_config.yml is set strictly to 0.0. This has also fixed the red None field for Characterisation tab.

fabcor-maxiv

This still seems to include changes (whole commits?) that have already been merged. The pull request contains 159 commits! To me this is usually a hint that some git operations (rebase maybe?) did not go well. I recommend to take some time to investigate and fix this. Maybe make sure to extract your actual changes from the ones that are merged upstream, then squash some commits to get the number down from 159.

Now for review itself I will only look at what is outside of the DESY directory.

fabcor-maxiv · 2023-12-06T16:04:15Z

mxcubecore/Command/Tango.py

This seems to contain only cosmetic changes. Do we want such changes in a pull request for a file that is otherwise untouched? What is the policy? @marcus-oscarsson

fabcor-maxiv · 2023-12-06T16:05:31Z

.github/workflows/tests.yml

This change was already merged in be49e6d

fabcor-maxiv · 2023-12-06T16:10:47Z

mxcubecore/HardwareObjects/abstract/AbstractSampleChanger.py

+                self.log.debug(f" Found sample {smp} is loaded")
+                self.log.debug(f"   getting loaded {self.get_loaded_sample()}")


As far as I know, the recommendation for logging in Python is to always use the "printf style formatting" (with % interpolation): https://docs.python.org/3/library/stdtypes.html#old-string-formatting

This is true. The best to satisfy both python recommendations and pylint would be to change to:

msg = f" Found sample {smp} is loaded\n getting loaded {self.get_loaded_sample()}" self.log.debug(msg)

Does pylint complain about printf style formatting for logging? It shouldn't, should it?

Unfortunately it does both - complains about not using f-string (which is in the log) and complains when using f-string in log. A bit of a mess, really.

https://pylint.pycqa.org/en/stable/user_guide/messages/warning/logging-format-interpolation.html

Using pylint 2.15.4

Is this okay?

self.log.debug("Found sample %s is loaded", smp) self.log.debug(" getting loaded %s", self.get_loaded_sample())

Seems fine for me with the same Pylint version 2.15.4 and the same .pylintrc configuration file.

…omatically after data collection.

marcus-oscarsson · 2024-01-22T13:47:11Z

Iam sorry @agruzinov I lost track of things, is this PR still active ?

agruzinov · 2024-01-23T16:00:05Z

In principle yes. I guess the main issue for now is the some problems with rebase. Maybe it is better to close it for now and open a new PR then??

agruzinov · 2024-02-20T10:54:31Z

PR needs to be resubmitted. Closing.

beteva · 2024-02-20T13:56:00Z

mxcubecore/HardwareObjects/abstract/AbstractMotor.py

+        if self._updating_value:
+            return  # Already updating, avoid recursion
+
+        self._updating_value = True
+


@agruzinov, could you tell us, please, where in the sample changer you get recursion ?

@beteva It looks like you are reviewing a closed pull request. If I understood correctly a new pull request has been created for the same change(s): #854

beteva · 2024-02-20T13:59:32Z

mxcubecore/HardwareObjects/DESY/P11Beam.py

+        self.focus_sizes = {
+            -1: {"label": "unknown", "size": (0.2, 0.2)},
+            0: {"label": "flat", "size": (0.2, 0.2)},
+            1: {"label": "200x200", "size": (0.2, 0.2)},
+            2: {"label": "100x100", "size": (0.1, 0.1)},
+            3: {"label": "50x50", "size": (0.05, 0.05)},
+            4: {"label": "20x20", "size": (0.02, 0.02)},
+            5: {"label": "4x9", "size": (0.009, 0.004)},


You have this both in DigitalZoomMotor and here. I am afraid this is prone to errors. Is it not possible to have it only in one place and get it in the other Hardware object

Andrey Gruzinov added 23 commits September 18, 2023 12:11

Fixing the ambiguity with Poller value and .any

e80379d

P11Collect additional debug after upgrade

5f29dc6

Fixing the ambiguity with Poller value and .any

d2b8c35

P11Collect additional debug after upgrade

ba7d34c

Merge remote-tracking branch 'origin/p11_develop' into p11_develop

9585b03

Force upgrade from the develop to avoid merging conflict

df5dfd3

Added missing warnings module

de981e7

Enabled back explicit check on if beamtimeid is open

24c07c7

Added debug info in case beamtime ID is not opened

d22d705

Added beamtime info as a property

4a38247

Added additional debug messages for shutter

480ec3e

Fixed get_properties naming (Bixente)

56d2cad

Removed faulty login_type=user fot P11

a07b2ec

Removed is_open() as a function and assigned is_closed() as a propert…

be6c170

…y for shutter-like objects inside P11 HO (Bixente)

Added is_open as a property inside P11NanoDiff (Bixente)

3e58e8d

Fixed other get_properties typos which were preventing loading

2a6a21e

Removed faulty login_type=user for P11

5d7ca11

Removed is_open() as a function and assigned is_closed() as a propert…

c20ec81

…y for shutter-like objects inside P11 HO (Bixente)

Added is_open as a property inside P11NanoDiff (Bixente)

7574477

Fixed other get_properties typos which were preventing loading

b4b8f9b

Merge remote-tracking branch 'origin/p11_develop' into p11_develop

4a192df

Fixed shutter openning wainting forever (Bixente)

f3bd7d5

Merge remote-tracking branch 'origin/p11_develop' into p11_develop

6fe81ab

fabcor-maxiv reviewed Oct 5, 2023

View reviewed changes

Andrey Gruzinov added 3 commits October 6, 2023 17:32

Added preliminary flux HO for P11

75621a3

Upgraded the characterisation part for P11 using EDNA

30ddf3a

Linting

e878dfb

beteva requested changes Oct 7, 2023

View reviewed changes

fabcor-maxiv requested changes Oct 12, 2023

View reviewed changes

Andrey Gruzinov added 15 commits November 15, 2023 15:20

Temp fix for P11 specific routine

0f434cb

Renamed get_properties to get_properties_ as before and made appropri…

e3fe513

…ate changes in qt part-multi_state_brick.py (PR800)

Cleanup unused code (PR800)

50c6b83

Remove conflicts

8dcf8b4

Linting

1c872e6

Linting

46b4fe5

Removed unused file

dfb0cb4

Removed unused DigitalZoomMotor.py

13bfad3

Removed unused Centring.py

85c4862

Put back deleted by mistake class

85bc098

Added guard variable to avoind recirsion error after sample loading i…

f03f779

…n TangoMotor.py

Bugfix for osc_start always initialised as None in case default param…

37b6915

…s in the beamline_config.yml is set strictly to 0.0. This has also fixed the red None field for Characterisation tab.

Fixed add_h5_info function in case the file is already existing

b35f10f

Changed path conversion in P11Characterisation.py

31cd0b8

Modified the way additional shutter values are handeled (PR800)

1bba3ed

agruzinov requested review from beteva and fabcor-maxiv December 2, 2023 19:28

agruzinov marked this pull request as ready for review December 2, 2023 19:29

Andrey Gruzinov added 2 commits December 2, 2023 20:34

Run black

8686c65

Disabled additional logging

c3a2a03

fabcor-maxiv requested changes Dec 6, 2023

View reviewed changes

Andrey Gruzinov added 2 commits December 12, 2023 15:38

Remove unnessesary triggerring of autoprocessing. It is triggered aut…

3f63381

…omatically after data collection.

Set omega to 0 at the end explicitly

1a0b15e

Merge branch 'mxcube:develop' into p11_develop

44d8f09

agruzinov closed this Feb 20, 2024

beteva reviewed Feb 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P11 develop #800

P11 develop #800

agruzinov commented Oct 5, 2023

fabcor-maxiv Oct 5, 2023

agruzinov Nov 8, 2023

beteva Oct 7, 2023 •

edited

Loading

beteva Oct 7, 2023

agruzinov Nov 8, 2023

beteva Nov 8, 2023

agruzinov Nov 8, 2023

agruzinov Nov 8, 2023

beteva Oct 7, 2023 •

edited

Loading

beteva commented Oct 7, 2023

fabcor-maxiv left a comment

fabcor-maxiv left a comment

fabcor-maxiv Dec 6, 2023

fabcor-maxiv Dec 6, 2023

fabcor-maxiv Dec 6, 2023

beteva Dec 6, 2023

fabcor-maxiv Dec 6, 2023

beteva Dec 6, 2023

fabcor-maxiv Dec 6, 2023

beteva Dec 6, 2023

fabcor-maxiv Dec 6, 2023

marcus-oscarsson commented Jan 22, 2024

agruzinov commented Jan 23, 2024

agruzinov commented Feb 20, 2024

beteva Feb 20, 2024 •

edited

Loading

fabcor-maxiv Feb 20, 2024

beteva Feb 20, 2024

		self.log.debug(f" Found sample {smp} is loaded")
		self.log.debug(f" getting loaded {self.get_loaded_sample()}")

P11 develop #800

P11 develop #800

Conversation

agruzinov commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beteva Oct 7, 2023 • 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

beteva Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

beteva commented Oct 7, 2023

fabcor-maxiv left a comment

Choose a reason for hiding this comment

fabcor-maxiv left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcus-oscarsson commented Jan 22, 2024

agruzinov commented Jan 23, 2024

agruzinov commented Feb 20, 2024

beteva Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beteva Oct 7, 2023 •

edited

Loading

beteva Oct 7, 2023 •

edited

Loading

beteva Feb 20, 2024 •

edited

Loading