Skip to content
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

[fix] Import Image and Audio for TensorboardFolderTracker #2669

Merged
merged 9 commits into from May 4, 2023

Conversation

alansaul
Copy link
Contributor

  • Add Image and Audio to be always imported
  • Added some tests as well - utilising some of torch's utilities.

Resolves #2668

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2023

CLA assistant check
All committers have signed the CLA.

tests/README.md Outdated Show resolved Hide resolved
@alansaul alansaul changed the title Import Image and Audio for TensorboardFolderTracker [fix]: Import Image and Audio for TensorboardFolderTracker Apr 21, 2023
@mihran113 mihran113 changed the title [fix]: Import Image and Audio for TensorboardFolderTracker [fix] Import Image and Audio for TensorboardFolderTracker Apr 23, 2023
@mihran113
Copy link
Contributor

@alansaul thanks a lot for contribution!
Everything looks fine, just a small comment there please sign the CLA and also would be nice to add a line in CHANGELOG as well. (under the unreleased section)

Copy link
Member

@alberttorosyan alberttorosyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Waiting for the CHANGELOG update to approve changes.

Copy link
Contributor

@mihran113 mihran113 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alansaul looks great! 🎉
Thanks a lot!
Please sign the CLA so I can merge the PR.

@alberttorosyan
Copy link
Member

@alansaul, just a reminder to sign the CLA, as it blocks the PR merge.
If there are any issues with that, please let me know, so we can proceed forward.

@gorarakelyan
Copy link
Contributor

@alansaul could you please sign the CLA, so we can merge the fix?

@alansaul
Copy link
Contributor Author

alansaul commented May 4, 2023

Apologies for the delay, I've merged upstream and signed the CLA, and fixed the changelog

@alansaul
Copy link
Contributor Author

alansaul commented May 4, 2023

It looks like this PR has tripped your coverage threshold of 50%. I don't think this PR should be further tested though, as the bulk of the PR itself is additional tests that didn't already exist, rather than code that needs to be tested.

@alberttorosyan
Copy link
Member

It looks like this PR has tripped your coverage threshold of 50%. I don't think this PR should be further tested though, as the bulk of the PR itself is additional tests that didn't already exist, rather than code that needs to be tested.

Yes, @alansaul, the coverage drop is a result of other changes. Seems the pipeline failed on this PR after to synching it with main. I'm going to address low coverage by a separate PR, so it should be fine to merge this one now.
Thanks again for your contribution!

@alberttorosyan alberttorosyan merged commit e5f615d into aimhubio:main May 4, 2023
3 of 4 checks passed
mihran113 pushed a commit that referenced this pull request May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TensorboardFolderTracker missing Image import
5 participants