Lidar parsing #50
Lidar parsing #50
Conversation
There was a problem hiding this comment.
Reviewed.
I think we can make an optimization to how we return a complete oscillation. instead of creating a LidarOscillation in a separate function and assigning it to a variable (extra memory) we can directly return it inside of process_reading(). i.e. if ... : return lidar_oscillation.LidarOscillation.create(self.lidar_readings) and then reinitialize lidar_readings
Also, it seems like run is just a wrapper for process reading so we can just move the contents of process_reading into the run function
|
|
||
| from modules import lidar_detection | ||
| from modules import lidar_oscillation | ||
| from modules import decision |
There was a problem hiding this comment.
remove decision import - not needed here
There was a problem hiding this comment.
can you also refer to https://uwarg-docs.atlassian.net/wiki/spaces/CV/pages/2253226033/Python+Style+Convention#Imports for import order
modules/lidar_parser/lidar_parser.py
Outdated
| Module to parse LiDAR data, detect oscillations, and return oscillation objects. | ||
| """ | ||
|
|
||
| from modules import lidar_oscillation |
There was a problem hiding this comment.
small nit - put this in alphabetical order
modules/lidar_parser/lidar_parser.py
Outdated
| __create_key = object() | ||
|
|
||
| @classmethod | ||
| def create(cls) -> "tuple[bool, LidarParser | None]": |
There was a problem hiding this comment.
no need for a create method for this module
modules/lidar_parser/lidar_parser.py
Outdated
| """ | ||
| Process a single LidarDetection. | ||
| """ | ||
| if not self.__run: |
There was a problem hiding this comment.
why do we need this check? the run function is called in a loop not just once
modules/lidar_parser/lidar_parser.py
Outdated
| self.current_oscillation = None | ||
|
|
||
| self.last_angle = None | ||
| self.direction = None |
There was a problem hiding this comment.
let's create an enum class for the direction of the lidar scan
| Feeding LidarParser continuously with a stream of LidarDetection | ||
| """ | ||
|
|
||
| result, parser = lidar_parser.LidarParser.create() |
| if lidar_data is None: | ||
| break | ||
|
|
||
| parser.run(lidar_data) |
There was a problem hiding this comment.
this should return a result and either none or a lidar osciallation
|
|
||
| parser.run(lidar_data) | ||
|
|
||
| result, oscillation = parser.run(detection_in_queue) |
There was a problem hiding this comment.
the run function accepts a LidarDetection as a parameter. so we don't pass the queue to the run function.
| Feeding LidarParser continuously with a stream of LidarDetection | ||
| """ | ||
|
|
||
| result, parser = lidar_parser.LidarParser() |
There was a problem hiding this comment.
this doesn't return a result - just the parser
| controller.check_pause() | ||
|
|
||
| lidar_reading: lidar_detection.LidarDetection = detection_in_queue.queue.get() | ||
| if lidar_reading is None: # Do we want this line? |
modules/lidar_parser/lidar_parser.py
Outdated
| Process a single LidarDetection and return the oscillation if complete. | ||
| """ | ||
|
|
||
| if lidar_detection is None: |
There was a problem hiding this comment.
worker ensures lidar_detection is not None
| controller: worker_controller.WorkerController, | ||
| ) -> None: | ||
| """ | ||
| Feeding LidarParser continuously with a stream of LidarDetection |
tests/unit/test_lidar_parser.py
Outdated
| from modules import lidar_detection | ||
| from modules.lidar_parser import lidar_parser | ||
|
|
||
| ANGLE_UP = 15.0 |
There was a problem hiding this comment.
these can just be hard-coded
| lidar_parser_instance.run(lidar_detection_up) | ||
| lidar_parser_instance.run(higher_angle_detection_up) | ||
| result, oscillation = lidar_parser_instance.run(lidar_detection_down) | ||
| assert result |
There was a problem hiding this comment.
check list length (i.e. len == 2) and check whether the oscillation contains only the readings going up i.e. it sees 15 and 20 degrees. (lidar_oscillation.readings[0].angle == 15)
There was a problem hiding this comment.
- expected = lidar_detection.create(5, 10)
- oscillation = parser.run()
- assert expected.angle == oscillation.readings[0].angle
There was a problem hiding this comment.
I've implemented this in the test_oscillation_detected_up_down
tests/unit/test_lidar_parser.py
Outdated
| result, oscillation = lidar_parser_instance.run(lidar_detection_down) | ||
| assert result | ||
| assert oscillation is not None | ||
| assert len(lidar_parser_instance.lidar_readings) == 0 |
There was a problem hiding this comment.
store last reading after direction switch
|
|
||
| for i in range(4): | ||
| if i % 2 == 0: | ||
| result, oscillation = lidar_parser_instance.run(higher_angle_detection_up) |
| @@ -188,6 +187,7 @@ def test_no_oscillation_with_single_reading( | |||
| result, oscillation = lidar_parser_instance.run(lidar_detection_up) | |||
| assert not result | |||
| assert oscillation is None | |||
| assert lidar_parser_instance.lidar_readings[0].angle == 15.0 | |||
There was a problem hiding this comment.
we want to check that the returned oscillation has the angle 15 in it not the instance which is the test case that you created.
There was a problem hiding this comment.
oscillation not being returned here, as solo angle being provided to our lidar_parser instance. so the last assert basically looks at the leftovers, that the 15 degree ange is being processed, but not returned
tests/unit/test_lidar_parser.py
Outdated
| @@ -102,6 +99,8 @@ def test_oscillation_detected_up_to_down( | |||
| result, oscillation = lidar_parser_instance.run(lidar_detection_down) | |||
| assert result | |||
| assert oscillation is not None | |||
| assert len(oscillation.readings) == 2 | |||
| assert lidar_parser_instance.lidar_readings[0].angle == -15.0 | |||
There was a problem hiding this comment.
check that the oscillation has this angle in it, not the instance that you created
Added the lidar_parser module