Replace (broken) --unrag with --ragged
#1539
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1539 +/- ##
===========================================
- Coverage 73.39% 73.39% -0.01%
===========================================
Files 134 134
Lines 23961 23961
===========================================
- Hits 17586 17585 -1
- Misses 6375 6376 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
WalkthroughThe changes revolve around the handling of ragged tensors in the CLI and Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- docs/guides/cli.md (1 hunks)
- sleap/nn/inference.py (2 hunks)
- tests/nn/test_inference.py (15 hunks)
Files skipped from review due to trivial changes (1)
- tests/nn/test_inference.py
Additional comments (Suppressed): 2
sleap/nn/inference.py (2)
4939-4944: The argument
unrag_outputshas been replaced withraggedand the logic has been inverted. Ensure that all calls toexport_modelfunction throughout the codebase have been updated to match the new argument and its behavior.4974-4981: The
--unragargument has been replaced with--raggedand the default value has been changed fromTruetoFalse. This changes the behavior of the command-line interface. Make sure that this change is communicated to the users and the documentation is updated accordingly.
docs/guides/cli.md
Outdated
| -e [EXPORT_PATH], --export_path [EXPORT_PATH] | ||
| Path to output directory where the frozen model will be exported to. | ||
| Defaults to a folder named 'exported_model'. | ||
| -u, --unrag UNRAG | ||
| Convert ragged tensors into regular tensors with NaN padding. | ||
| Defaults to True. | ||
| -r, --raggged RAGGED | ||
| Keep tensors tensors ragged if present. If ommited, convert | ||
| ragged tensors into regular tensors with NaN padding. | ||
| -n, --max_instances MAX_INSTANCES | ||
| Limit maximum number of instances in multi-instance models. | ||
| Not available for ID models. Defaults to None. |
There was a problem hiding this comment.
The new -r or --ragged option is well explained. However, there are two typos in line 102 and 103. The word "ragged" is misspelled in line 102 and the word "tensors" is repeated in line 103. Here is the corrected version:
- 102: -r, --raggged RAGGED
+ 102: -r, --ragged RAGGED
- 103: Keep tensors tensors ragged if present. If ommited, convert
+ 103: Keep tensors ragged if present. If omitted, convert 
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- docs/guides/cli.md (1 hunks)
- sleap/nn/inference.py (2 hunks)
Files skipped from review due to trivial changes (1)
- docs/guides/cli.md
Additional comments (Suppressed): 2
sleap/nn/inference.py (2)
4939-4944: The function call to
export_modelhas been updated to use the newargs.raggedargument instead of the oldargs.unragargument. The logic has been inverted withnot args.raggedto maintain the original behavior. Ensure that theargs.raggedargument is correctly set in the calling context.4974-4982: The
-u/--unragargument has been replaced with-r/--ragged. The default value has been changed fromTruetoFalse, and the help message has been updated accordingly. Make sure that all instances of-u/--unragin the codebase and documentation have been replaced with-r/--ragged.
|
Although not "official", this review was discussed and verbally approved on Tuesday's meeting, so merging. |
Description
The
--unragargument was previous unable to be set toFalsedue to its "default of true" and "store_true" nature. This PR flips to a more correct logic which uses a--raggedargument with default false and true if--raggedor-ris included (i.e. store_true). The broken--unragargument has been dropped. This is an alternative PR to #1531.Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
-u/--unragoption with-r/--raggedin the command-line interface. The new-roption allows keeping tensors ragged if present. If omitted, ragged tensors will be converted into regular tensors with NaN padding.test_make_export_clito verify the functionality of the updated command-line parser.