-
Notifications
You must be signed in to change notification settings - Fork 1
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
Train-Only Heads #83
Train-Only Heads #83
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## dev #83 +/- ##
======================================
Coverage ? 97.07%
======================================
Files ? 140
Lines ? 6120
Branches ? 0
======================================
Hits ? 5941
Misses ? 179
Partials ? 0 ☔ View full report in Codecov by Sentry. |
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.
Left a small comment, otherwise LGTM.
However, it would be good to add a simple test case for a dummy network with a train-only head:
model = ...
results = model.eval()
# test results contain all heads
output_names = model.export_onnx()
# test output_names don't contain the train-only heads
luxonis_train/nodes/base_node.py
Outdated
"""Getter for the remove_on_export attribute.""" | ||
return self._remove_on_export | ||
|
||
def set_remove_on_export_mode(self, mode: bool = True) -> 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.
I don't think the setter is necessary. There shouldn't be a need to change the value once it's set, and if a niche use case arises, we can rely on the underlying _remove_on_export
.
IMHO, implementing a setter suggests that it's reasonable for the user to change the once-set value, which isn't the case here. Instead, making it accessible only via the original _remove_on_export
attribute indicates that the value shouldn't be modified in regular use cases.
If we'd want to keep the setter anyway, I think implementing it as an actual setter:
@remove_on_export.setter
def remove_on_export(self, mode: bool) -> None:
...
is a bit nicer.
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.
I agree, I don't think we need setter for this. I don't see it being changed after init in 99% of the cases
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.
Thanks, LGTM!
Luxonis Train - Remove on Export Feature
The
remove_on_export
option allows users to exclude specific nodes during model export. This is useful for auxiliary nodes that are not required in the final exported model.Example Usage:
Nodes marked with
remove_on_export: true
will be skipped when exporting the model.