Continual Transformer Encoder #317
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LukasHedegaard
requested review from
passalis,
omichel,
ad-daniel and
stefaniapedrazzi
as code owners
LukasHedegaard
added
enhancement
…opendr into continual-transformer
|
I assume this PR is still ongoing, I've converted it to draft for now |
It is ready for review |
ad-daniel
requested changes
Oct 25, 2022
There was a problem hiding this comment.
Thank you, only a few points
- I've already committed some formatting adjustments.
- ensure the default values in the code are indeed the desired ones and adapt the docs/comments accordingly
- add a changelog entry mentioning the added tool
- if possible, provide a demo that shows how to use it (like you did in the docs, can be the same one)
- Concerning the project structure, my understanding is that it's a self-contained module. You've made (rightly so) an ad hoc folder for this new tool in perception/activity_recognition and test folders, but documentation wise you've put everything in the same existing document. I think it would be better to split the documentation of x3d and this tool into 2 separate documents. Basically as we did for other stuff like speech recognition (see link), and each document should be referenced in the index here : https://github.com/opendr-eu/opendr/blob/master/docs/reference/index.md
- I don't think this was among the planned tools, so there isn't a strict requirement, but having a ROS/ROS2 node would be a plus. Up to you.
projects/python/perception/activity_recognition/benchmark/README.md
Outdated
Show resolved
Hide resolved
.../activity_recognition/continual_transformer_encoder/continual_transformer_encoder_learner.py
Show resolved
Hide resolved
Co-authored-by: ad-daniel <[email protected]>
|
My bad. I've updated the CHANGELOG and added a demo. I will not be providing a ROS node at this time. |
ad-daniel
requested changes
Dec 7, 2022
Closed
Likewise :-) |
tsampazk
requested changes
Dec 7, 2022
projects/python/perception/activity_recognition/demos/continual_transformer_encoder/README.md
Outdated
Show resolved
Hide resolved
projects/python/perception/activity_recognition/demos/continual_transformer_encoder/README.md
Outdated
Show resolved
Hide resolved
.../activity_recognition/continual_transformer_encoder/continual_transformer_encoder_learner.py
Outdated
Show resolved
Hide resolved
.../activity_recognition/continual_transformer_encoder/continual_transformer_encoder_learner.py
Show resolved
Hide resolved
.../activity_recognition/continual_transformer_encoder/continual_transformer_encoder_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/activity_recognition/datasets/dummy_timeseries_dataset.py
Outdated
Show resolved
Hide resolved
…l_transformer_encoder/README.md Co-authored-by: Kostas Tsampazis <[email protected]>
…l_transformer_encoder/README.md Co-authored-by: Kostas Tsampazis <[email protected]>
…opendr into continual-transformer
Co-authored-by: Kostas Tsampazis <[email protected]>
Co-authored-by: Kostas Tsampazis <[email protected]>
Co-authored-by: Kostas Tsampazis <[email protected]>
…opendr into continual-transformer
…er_encoder/continual_transformer_encoder_learner.py Co-authored-by: Kostas Tsampazis <[email protected]>
…opendr into continual-transformer
Thanks for the thorough review, @tsampazk . I've accepted all your suggestions and fixed typos as requested. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New tool for efficient application of Transformer Encoders to time-series data.
Note: this does not come with pre-trained models. The user must fit the learner on her own features.