-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_outputs
has been replaced withragged
and the logic has been inverted. Ensure that all calls toexport_model
function throughout the codebase have been updated to match the new argument and its behavior.4974-4981: The
--unrag
argument has been replaced with--ragged
and the default value has been changed fromTrue
toFalse
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_model
has been updated to use the newargs.ragged
argument instead of the oldargs.unrag
argument. The logic has been inverted withnot args.ragged
to maintain the original behavior. Ensure that theargs.ragged
argument is correctly set in the calling context.4974-4982: The
-u
/--unrag
argument has been replaced with-r
/--ragged
. The default value has been changed fromTrue
toFalse
, and the help message has been updated accordingly. Make sure that all instances of-u
/--unrag
in 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
--unrag
argument was previous unable to be set toFalse
due to its "default of true" and "store_true" nature. This PR flips to a more correct logic which uses a--ragged
argument with default false and true if--ragged
or-r
is included (i.e. store_true). The broken--unrag
argument 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
/--unrag
option with-r
/--ragged
in the command-line interface. The new-r
option allows keeping tensors ragged if present. If omitted, ragged tensors will be converted into regular tensors with NaN padding.test_make_export_cli
to verify the functionality of the updated command-line parser.