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

Loss Mean Squared Logarithmic error. #95

Merged
merged 19 commits into from
Sep 30, 2020
Merged

Conversation

abhinavsp0730
Copy link
Contributor

@abhinavsp0730 abhinavsp0730 commented Sep 24, 2020

  • Added loss

  • Added test cases

  • Formatted the code with black

  • Added the documention

@codecov-commenter
Copy link

Codecov Report

Merging #95 into master will increase coverage by 0.26%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   71.27%   71.53%   +0.26%     
==========================================
  Files          91       93       +2     
  Lines        4055     4100      +45     
==========================================
+ Hits         2890     2933      +43     
- Misses       1165     1167       +2     
Impacted Files Coverage Δ
...legy/losses/mean_squared_logarithmic_error_test.py 93.10% <93.10%> (ø)
elegy/losses/__init__.py 100.00% <100.00%> (ø)
elegy/losses/mean_squared_logarithmic_error.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d433556...aba2612. Read the comment docs.

@abhinavsp0730
Copy link
Contributor Author

Hi, @charlielito please review.
Thanks

Copy link
Collaborator

@charlielito charlielito left a comment

Choose a reason for hiding this comment

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

Looks really good!

Only one thing, from now on we are making the tests also comparing with tensorflow's implementation like in this test: https://github.com/poets-ai/elegy/blob/master/elegy/losses/binary_crossentropy_test.py#L36

Also we made today some changes to the auto docs, please run again bash scripts/run-docs.sh

@abhinavsp0730
Copy link
Contributor Author

abhinavsp0730 commented Sep 28, 2020

Hy, @charlielito I've done all the changes suggested by you. Would you please review again. Thanks.

Copy link
Collaborator

@charlielito charlielito left a comment

Choose a reason for hiding this comment

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

LGTM

@charlielito charlielito merged commit 014647a into poets-ai:master Sep 30, 2020
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.

None yet

3 participants