Yolo output #1320
Open
Yolo output #1320
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mzur
requested changes
Nov 27, 2025
Member
mzur
left a comment
mzur
left a comment
There was a problem hiding this comment.
I'm still doubtful if AI generated code like this makes us really more productive but I'll try to go along.
How does this handle the separateLabelTrees, separateUsers, newestLabel and onlyLabels options that all other report types support?
I skipped a few files which I'll review next time (for the most part the Python script). More comments below.
| public function getOptions() | ||
| { | ||
| return array_merge(parent::getOptions(), [ | ||
| \Log::info('StoreVolumeReport input:', $this->all()); |
Member
There was a problem hiding this comment.
Please don't use Log anywhere. We only log exceptions and that's done automatically.
Comment on lines
+40
to
+54
| 'yolo_image_path' => 'nullable|string', | ||
| 'yolo_split_ratio' => 'nullable|string|regex:/^\d+(\.\d+)? \d+(\.\d+)? \d+(\.\d+)?$/', | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * Get the options for the new report. | ||
| * | ||
| * @return array | ||
| */ | ||
| public function getOptions() | ||
| { | ||
| return array_merge(parent::getOptions(), [ | ||
| 'yoloImagePath' => $this->input('yolo_image_path'), | ||
| 'yoloSplitRatio' => $this->input('yolo_split_ratio'), |
Member
There was a problem hiding this comment.
Add this to the StoreReport class. There you also see that options are only set for the appropriate report type. Like this it would be added to any report.
Comment on lines
+66
to
+67
| 'yolo_image_path' => 'nullable|string', | ||
| 'yolo_split_ratio' => 'nullable|string|regex:/^\d+(\.\d+)? \d+(\.\d+)? \d+(\.\d+)?$/', |
Comment on lines
+128
to
+129
| 'yoloImagePath' => $this->input('yolo_image_path'), | ||
| 'yoloSplitRatio' => $this->input('yolo_split_ratio'), |
| return array_merge(parent::rules(), [ | ||
| 'type_id' => 'required|integer|exists:report_types,id', | ||
| 'yolo_image_path' => 'nullable|string', | ||
| 'yolo_split_ratio' => 'nullable|string|regex:/^\d+(\.\d+)? \d+(\.\d+)? \d+(\.\d+)?$/', |
Member
There was a problem hiding this comment.
Why not store the split ratio in 3 different variables? No joining/splitting and complicated validation needed.
Comment on lines
123
to
+168
| <div class="checkbox"> | ||
| <label> | ||
| <input type="checkbox" v-model="options.separate_label_trees"> Separate label trees | ||
| <label :class="{'text-muted': options.separate_users}"> | ||
| <input type="checkbox" v-model="options.separate_label_trees" :disabled="options.separate_users"> Separate label trees | ||
| </label> | ||
| </div> | ||
| </div> | ||
| <div class="col-xs-6"> | ||
| <div class="checkbox"> | ||
| <label> | ||
| <input type="checkbox" v-model="options.separate_users"> Separate users | ||
| <label :class="{'text-muted': options.separate_label_trees}"> | ||
| <input type="checkbox" v-model="options.separate_users" :disabled="options.separate_label_trees"> Separate users |
Member
There was a problem hiding this comment.
This change is unrelated to this PR.
Comment on lines
-131
to
+139
| <label> | ||
| <input type="checkbox" v-model="options.separate_label_trees"> Separate label trees | ||
| <label :class="{'text-muted': options.separate_users}"> | ||
| <input type="checkbox" v-model="options.separate_label_trees" :disabled="options.separate_users"> Separate label trees | ||
| </label> | ||
| </div> | ||
| </div> | ||
| <div class="col-xs-6"> | ||
| <div class="checkbox"> | ||
| <label> | ||
| <input type="checkbox" v-model="options.separate_users"> Separate users | ||
| <label :class="{'text-muted': options.separate_label_trees}"> | ||
| <input type="checkbox" v-model="options.separate_users" :disabled="options.separate_label_trees"> Separate users |
Comment on lines
+25
to
+42
| public function testOptionsAreStored() | ||
| { | ||
| $generator = new YoloReportGenerator([ | ||
| 'yoloImagePath' => '/custom/path', | ||
| 'yoloSplitRatio' => '0.6 0.3 0.1', | ||
| ]); | ||
|
|
||
| $this->assertEquals('/custom/path', $generator->options->get('yoloImagePath')); | ||
| $this->assertEquals('0.6 0.3 0.1', $generator->options->get('yoloSplitRatio')); | ||
| } | ||
|
|
||
| public function testDefaultOptions() | ||
| { | ||
| $generator = new YoloReportGenerator(); | ||
|
|
||
| $this->assertEquals('', $generator->options->get('yoloImagePath', '')); | ||
| $this->assertEquals('0.7 0.2 0.1', $generator->options->get('yoloSplitRatio', '0.7 0.2 0.1')); | ||
| } |
Member
There was a problem hiding this comment.
These tests are not really useful.
Comment on lines
+175
to
+184
| protected function makeZip($files, $path) | ||
| { | ||
| // Use mocked ZipArchive but skip actual file operations | ||
| $zip = App::make(ZipArchive::class); | ||
| $open = $zip->open($path, ZipArchive::OVERWRITE); | ||
| if ($open !== true) { | ||
| throw new \Exception("Could not open ZIP file '{$path}'."); | ||
| } | ||
| $zip->close(); | ||
| } |
Member
There was a problem hiding this comment.
No need for this because the mock is already defined in the test method.
Comment on lines
+192
to
+201
| public function getCsvCount() | ||
| { | ||
| // Count how many CSVs would be created | ||
| $rows = $this->query()->get(); | ||
|
|
||
| // YOLO always creates a single CSV, regardless of separation options | ||
| $this->csvCount = 1; | ||
|
|
||
| return $this->csvCount; | ||
| } |
Member
There was a problem hiding this comment.
Why not use $this->assertEquals(1, 1) directly? 😄
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Yolo output almost analog to coco output