-
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
Unify test_run_transformers* and test_eval_model* scripts #98
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
+ Coverage 10.76% 10.85% +0.08%
==========================================
Files 25 25
Lines 3872 3877 +5
==========================================
+ Hits 417 421 +4
- Misses 3455 3456 +1 ☔ View full report in Codecov by Sentry. |
@@ -99,6 +99,7 @@ | |||
from tabulate import tabulate | |||
from torch.nn.parallel import DistributedDataParallel | |||
|
|||
from deepdisc.astrodet import detectron as detectron_addons |
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.
Not a blocker - I'm not immediately seeing why this line was added.
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.
Wasn't sure if I should explicitly mention it (nice eye there)!
It should have been added in the process of combining various versions of train_mapper_cls
and moving it into astrodet. The previous versions had all lived in places that had this import, and I had overlooked that in the astrodet PR.
It comes up way below on L1675 and onwards as, say, augs = detectron_addons.KRandomAugmentationList(
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.
This all looks pretty reasonable to me. Thanks for putting it together!
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 think rather than using conditional statements in the script it would be better to place these conditional variables in a solo config. Mostly to better keep track of changes and minimize the clutter of the actual train script (I now see you've planned this in a follow-up PR)
test_run_transfomers.py
(_DC2.py
,_DC2_redshift.py
, etc) intoany_test_run_transformers.py
Combinations can be accessed via new
--use-dc2
and--use-redshift
flags.$bash run_all.sh
runs all combinations (including swin/mvitv2) and outputs torun_all.log
Other: