-
Notifications
You must be signed in to change notification settings - Fork 234
Fix: Allow dictionary format in augmentation config schema #1248
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for this, please could you provide test cases that would cover the changes? e.g. configs with (dict) augmentation listings that would currently fail. Also as the other PR, please could you also include the AI assistance declaration from the template? |
henrykironde
left a comment
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.
@musaqlain, We expect some tests for changes in logic.
Thank you for the contribution
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
+ Coverage 87.73% 87.83% +0.09%
==========================================
Files 20 20
Lines 2716 2720 +4
==========================================
+ Hits 2383 2389 +6
+ Misses 333 331 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thanks, I have added the requested test case in |
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 for adding that! In the test case, please pass the dict to utilities.load_config or as a config_arg to main.deepforest.
The current test assigns the dict after instantiation and I'm not sure if the schema is re-validated? Maybe it does, but it's a bit clearer if we check how the code would normally be used (ideally we should set up the config once and not modify it).
|
Thanks for the feedback! You are right, testing it during initialization is much safer to ensure validation triggers correctly. I have updated it. |
jveitchmichaelis
left a comment
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.
@musaqlain thanks, I will take a closer look this week. I added a couple more suggestions, while we're adding this fix we could take a moment to improve the tests more.
The test should cover some other variations that we might typically expect; for example it's valid to mix strings ("HorizontalFlip", to use defaults) and dicts (to customize parameters). Note that we do test the parser for this behaviour earlier in the file.
Another suggestion is to enforce stricter typing here, as Any is very broad. I need to check how OmegaConf handles unions, but we could try list[union[str, dict]] or something similar. I'm not sure if we would also have to add a specific type for the dict.
…e to OmegaConf limitation
|
Thanks for the detailed review! I've updated the PR with the following changes:
2. Schema Typing:
|
|
Hi, does it look good? I have updated the integration tests. can you please check now. cc @jveitchmichaelis @henrykironde |
Description
The current schema in
conf/schema.pywas restricted toList[str]. This causedomegaconf.errors.ValidationErrorwhen users tried to provide detailed augmentation configurations (dictionaries with parameters), as described in the issue.Changes Made
augmentationstype hint inTrainConfigandValidationConfigfromList[str]toList[Any].test_augmentation_schema_validationintests/test_augmentations.pyto ensure dictionary configurations are accepted.Testing
test_augmentation_schema_validationwhich explicitly initializes a model with a complex dictionary augmentation list (e.g.,RandomResizedCropwith params) and asserts it is stored correctly without raising a ValidationError.pytest tests/test_augmentations.pyto ensure existing string-based augmentations (like["HorizontalFlip"]) still pass.Linked Issue
Closes #1234
AI-Assisted Development
I utilized AI for sanity checking code logic and conducting an assisted review to identify potential missing resets and schema constraints.