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.

Sign up for GitHub

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

Jump to bottom

95 refactor unit tests #100

Merged
merged 7 commits into from
Dec 11, 2023
Merged

95 refactor unit tests #100

merged 7 commits into from
Dec 11, 2023

Conversation

rogerkuou
Copy link
Contributor

fix #95

@rogerkuou rogerkuou changed the base branch from main to generalization December 6, 2023 13:22
@rogerkuou
Copy link
Contributor Author

Hi @SarahAlidoost, when you have time, could you please also review this one for me? I improved the unit tests to increase the coverage and speed.

SarahAlidoost
SarahAlidoost reviewed Dec 6, 2023
View reviewed changes
motrainer/util.py Outdated Show resolved Hide resolved
SarahAlidoost
SarahAlidoost reviewed Dec 6, 2023
View reviewed changes
tests/test_dnn.py Outdated Show resolved Hide resolved
SarahAlidoost
SarahAlidoost requested changes Dec 6, 2023
View reviewed changes
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@rogerkuou nice work 👍 please see my comments. One question: You mentioned that this pull request improves the speed of the tests. I am just curious about how you measured it. how much improvement in the speed is reached?

@rogerkuou
Copy link
Contributor Author

Hi @SarahAlidoost, thanks for the review!
I applied most of your comments. Apart from the one on checking best_loss and history, see my comments. Could you please give another look?

@SarahAlidoost
Copy link
Member

Hi @SarahAlidoost, thanks for the review! I applied most of your comments. Apart from the one on checking best_loss and history, see my comments. Could you please give another look?

Thanks for addressing the comments, feel free to merge 👍

@SarahAlidoost SarahAlidoost mentioned this pull request Dec 11, 2023
Merged
3 tasks
@rogerkuou rogerkuou merged commit 73faf8c into generalization Dec 11, 2023
12 checks passed
@rogerkuou rogerkuou mentioned this pull request Dec 12, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SarahAlidoost SarahAlidoost SarahAlidoost requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unit tests
2 participants
@rogerkuou @SarahAlidoost