Add support for training trackastra with SAM2 features - #61
Conversation
|
Poof, some ruff linting structures are funny haha. All should be working now. Lemme know how it looks @C-Achard |
There was a problem hiding this comment.
Thanks @anwai98, had a quick look and the approach seems reasonable, if you end up requiring changes on the pretrained_feats repo happy to have a look as well.
One thing I noticed is that in train.py, if no model path is given (training from scratch), it will load the basic model from Trackastra, rather than the one from pretrained_feats, since in the inference-only version create() is called only from TrackingTransformer.from_folder and then it would likely crash due to the extra args.
Now you did mention you wanted to fine-tune only but maybe the best is to add some error handling if anyone tries to train a pretrained_feats model from scratch, since the resulting exception will likely look unclear if no guard is added.
Otherwise, I noticed some slightly misleading help strings in the CLI, perhaps have a look at the manuscript for better context on what these options do (I added comments on these with recommended defaults).
Finally, if your next step is to train a model, those previous configs may come in handy for that.
I hope this helps, I'm afraid I cannot test this extensively right now but happy to help further if anything is unclear in the review.
Best,
Cyril
Co-authored-by: Cyril Achard <cyril.achard@epfl.ch>
Co-authored-by: Cyril Achard <cyril.achard@epfl.ch>
Co-authored-by: Cyril Achard <cyril.achard@epfl.ch>
|
Hi @C-Achard, Thank you so much for the detailed feedback. I'll check them out later in the evening and come back to you! |
….com/anwai98/trackastra into add-training-support-with-sam2-feats
|
Hi @C-Achard, Sorry for the super late follow-up. I managed to come back to the PR this week only - had a couple of busy weeks in the past. I took care of the comments you left. What do you think about the current state now? |
Nice, thanks! This looks good as far as I can tell, definitely curious to see how this performs on your data. If it overfits too much I would look into the augmentation API (I can help) but this is likely not needed for a first check. Let me know if I can help with anything else, thanks again |
|
Hi @C-Achard, Super late update, sorry - the training pipeline works now, both finetuning from On the augmented copies: you were right that they're not generated, but I also managed a first real run: finetuning on two (TOIAM) 800-frame sequences (ca. 570 detections/window), validating on a third. No sign of overfitting so far. What do you think? @C-Achard |
|
Oh btw -- Claude noticed a bug in |
|
Ahha also spotted another issue in a round of review by Codex (also fixed it -- see f4bcd48): if |
|
Hi @anwai98, Thanks for the further updates, glad to hear that the finetuning seems to be working. Both issues you mentioned seem correctly reported to me, the feature rotation one is particularly important, thanks for finding that. I might have to rerun some of my offline analysis using the fixed version of the rotation to see if the initial conclusions regarding the disambiguation still hold; hopefully it only gets better from there.
If moving that is not too much work, it would definitely be great to have it. From what I recall we omitted it (and the training) to keep the results from the SAM2 models usable without creating too much maintenance overhead/API changes with training. But it would certainly make the PR more complete. I think it's the only important missing piece. Happy to help with the review of the implementation when I have time, let me know if you go ahead! Thanks again for trying this out and working on the API, really appreciate it. Best, |
|
Hi @C-Achard,
Nice, thanks for the affirmation.
Oh yeah, good point. By default keeping it fixed at the previous setup, and can be overridden to the expected version. I'll take care of it!
That would be great, thanks!
Gotcha. Then I'll give it a shot and create a PR in the other repo ;)
Ofc. And we have our interests vested in this anyways hehe, so we thought bringing this in a correct working structure would be awesome for us, and others! 😁 I'd probably touch the updates I promised above on Wednesday and ping you then! |
|
Hi @C-Achard, I opened a PR (C-Achard/Trackastra-et-Ultra#2) for the augmentation port. Also made the rotation axes configurable here, with defaults same as before. I also wired in What do you think about this now? |
There was a problem hiding this comment.
l. 554 is missing:
if features_type in ["none", "wrfeat"]:
Right n_workers and feats extraction logics are mixed
There was a problem hiding this comment.
Also "none" feature handling is missing it seems...
There was a problem hiding this comment.
(Note that these do not seem to be introduced by you directly, they may be older oversights)
There was a problem hiding this comment.
try:
PRETRAINED_FEATS_INSTALLED = True
if TYPE_CHECKING:
from trackastra_pretrained_feats import FeatureExtractor
except ImportError:
PRETRAINED_FEATS_INSTALLED = False
if TYPE_CHECKING:
FeatureExtractor = None # type: ignore
The bool can be removed, with type checking this is safe and the bool is not used here
|
Hello again @anwai98, Thanks for the update, and sorry for the late review. Overall the PR in the other repo looks good, just a few comments, but it is mainly the remaining changes on this repo that may be more difficult. A lot of the wiring was updated in the fork I used, https://github.com/C-Achard/trackastra/blob/cy/aug-zarr-caching/trackastra/data/wrfeat.py, e.g. when returning WRFeatures, So I think this is only the first step of a larger migration, I purposefully omitted several not-so-trivial integration changes when porting this the first time, trying to make a minimal inference-only version. None of this necessarily needs to be addressed as part of this PR, I just wanted to clarify the scope. Let me know if I can help :) Best, |
|
@anwai98 Thinking a bit about this, would it perhaps be easier for you to use the training code in my fork directly first? Fine-tuning should definitely be possible there, so you can at least try out the model with/without augmentations and this way determine whether refactoring here would be worth it? Maybe you already have preliminary results to motivate add the augmentation API here though, but since there was no overfitting in your latest round you may want to check first whether it helps |
|
Hi @C-Achard, Thanks for your response. I'll look at your comments and response closely later today. Quick re: on something:
Ofc, I'd be happy to. Then we can potentially have two parallels to draw conclusion from (hopefully not orthogonal haha) Will come back to you later with more thoughts! |
|
Let me know if you need anything that may be in my thesis archive as well (train scripts, config, etc), as I forget exactly what is and isn't in the fork! |
Hi @C-Achard,
Here's are my minimal changes to make training work with SAM2 features.
Let me know how it looks!
PS. In case it helps, here's my yaml config file to train
trackastra:yaml config