[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 #2757

DonghakPark · 2024-10-16T05:51:47Z

In this PR

In this PR, performed three tasks as follows:

commit 1 - [UnitTest] Add kld-loss unit test

To increase coverage, we added a KLD loss layer using LayerSemanticsParamType to the unit test case.

commit 2 - [UnitTest] Add integration_tests on unit test

Considering the characteristics of NNTrainer, it was difficult to accurately test one loss or BN layer, so we created integration tests and wrote exception cases.

commit 3 - [Loss] Implement KLD Loss Layer

implemented KLD Loss in torch
KLD_loss forwarding formula

Self evaluation:
1. Build test: [X]Passed [ ]Failed [ ]Skipped
2. Run test: [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Donghak PARK [email protected]

taos-ci · 2024-10-16T05:51:50Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2757. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

taos-ci

@DonghakPark, 💯 All CI checkers are successfully verified. Thanks.

SeoHyungjun · 2024-10-16T06:40:27Z

There is a typo in 'implemented KDL Loss layer as below'.
It should be corrected to 'implemented KLD Loss layer as below.'

SeoHyungjun · 2024-10-16T07:04:09Z

nntrainer/layers/loss/kld_loss_layer.cpp

+  mu.pow(2.0f, temp);                 // 1. temp = mu ^ 2
+  log_std.subtract(temp, before_sum); // 2. before_sum = log_std - temp
+  log_std.apply<float>(expf, temp);   // 3. temp = exp(log_std) - 1
+  temp.subtract_i(1.0f);
+  before_sum.subtract_i(temp);          // 4. before_sum = before_sum - temp
+  before_sum.sum({1, 2, 3}, ret, -0.5); // 5. sum * 0.5


I don't fully understand the forward implementation of KLD Loss in commit 3.
Are the written formulas and actual implementations the same?

I would like to know the difference between the formula on line 32 of kld_loss_layer.cpp and the PyTorch's KLD formula added to the PR.

In PyTorch, it is summarized as follows:

if not log_target: # default loss_pointwise = target * (target.log() - input) else: loss_pointwise = target.exp() * (target - input)

I should have provided more detailed explanations. Upon reviewing your feedback, it seems necessary to align with other frameworks' usage. Thank you for bringing this to my attention.

Eunju provided additional explanations about logic through the comment below.
I think it would be great if we make a decision with our team members based on that opinion!

SeoHyungjun · 2024-10-16T07:06:50Z

nntrainer/layers/loss/kld_loss_layer.cpp

+  mu.pow(2.0f, temp);                 // 1. temp = mu ^ 2
+  log_std.subtract(temp, before_sum); // 2. before_sum = log_std - temp
+  log_std.apply<float>(expf, temp);   // 3. temp = exp(log_std) - 1
+  temp.subtract_i(1.0f);
+  before_sum.subtract_i(temp);          // 4. before_sum = before_sum - temp
+  before_sum.sum({1, 2, 3}, ret, -0.5); // 5. sum * 0.5


The comment says "sum * 0.5".
However, in the code, "-0.5" is used.
Is this correct?

Nice Catch --> -0.5 is correct, at code i apply -0.5

SeoHyungjun · 2024-10-16T07:10:49Z

nntrainer/layers/loss/kld_loss_layer.cpp


-void KLDLossLayer::forwarding(RunLayerContext &context, bool training) {}
+void KLDLossLayer::forwarding(RunLayerContext &context, bool training) {
+  // -0.5 * sum(1 + log_std - pow(mu, 2) - exp(log_std))


If it is the same as torch's KLD formula, it would be great if you could write down the derivation process of the formula like in the comment for the calcDerivative function.

Okay i will fix

EunjuYang · 2024-10-16T10:22:23Z

nntrainer/layers/loss/kld_loss_layer.cpp


-void KLDLossLayer::forwarding(RunLayerContext &context, bool training) {}
+void KLDLossLayer::forwarding(RunLayerContext &context, bool training) {
+  // -0.5 * sum(1 + log_std - pow(mu, 2) - exp(log_std))


I agree with Hyungjun. Torch's KLD loss has their own assumptions : (1) reverse KL divergence (2) they are assuming the value from model takes log.

For general KL divergence,

# KL(P||Q) = \sum { P (log P - log Q) } (P * (P / Q).log()).sum()

How to use KLD loss of pytorch

# in general, kld loss is used in the form of torch.nn.functional.kl_div(Q.log(), P, None, None, 'sum')

Like this, we need to clarify the assumption on how to use this loss function -- including input definition, etc. In this update, these kind of explanations are missing. In order to review this PR correctly, some explanations are required.

Also, I couldn't understand why you used notation of mu and log_std; it is not equivalent with the general kl loss I think. Do you assume KLD loss for Gaussian? (e.g., VAE KL loss - it seems this KLD loss is a specific one to compute KLD btw/ normal distribution and standard normal)

Thank you for your detailed review. Based on the formula below,
https://stackoverflow.com/questions/74865368/kl-divergence-loss-equation

I have attached the Torch formula to assist in understanding. As Hyungjun and Eunju mentioned, it seems necessary to discuss which assumptions we need to implement this.

Thank you for your detailed explanation. As i mentioned in the comment, however, it seems the forwarding of KLD loss only covers the special case of $KL(p(x)||q(x))$, where $q(x) \sim \mathcal{N}(\mu , \sigma^2)$ and $p(x) \sim \mathcal{N}(0,1)$.
The link you shared refers the code of a page, which considers the special case of KL loss for training VAE, (here). I recommend you to consider updating this to make our KLD loss cover more general form :)

Oh, i see
i will update kld loss for more general cases

taos-ci · 2024-10-17T00:16:52Z

cibot: @DonghakPark, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2757-202410170844270.1929669380188-3475994cecf485dfba4d34fa1aeab25e0bb0695e/.

taos-ci

@DonghakPark, 💯 All CI checkers are successfully verified. Thanks.

EunjuYang

Thank you for considering my previous comment.
Please check some suggestions below :)

EunjuYang · 2024-10-23T01:50:53Z

nntrainer/layers/loss/kld_loss_layer.cpp

+    /**
+     * 1. Output = label / predicted
+     * 2. Output = output * label
+     * 3. Output = log(output)
+     * 4. Output = sum(output)
+     */
+    label.divide(predicted, temp);
+    temp.multiply_i(label);
+    temp.apply<float>(logf, temp);
+    output.fill(temp.sum({0, 1, 2, 3}));


The code seems like (P * (P / Q)).log() which is different from P * (P/Q).log().
Please change the code like

Suggested change

/**

* 1. Output = label / predicted

* 2. Output = output * label

* 3. Output = log(output)

* 4. Output = sum(output)

*/

label.divide(predicted, temp);

temp.multiply_i(label);

temp.apply<float>(logf, temp);

output.fill(temp.sum({0, 1, 2, 3}));

/**

* 1. Output = label / predicted

* 2. Output = log(Output)

* 3. Output = Output * label

* 4. Output = sum(output)

*/

label.divide(predicted, temp);

temp.apply<float>(logf, temp);

temp.multiply_i(label);

output.fill(temp.sum({0, 1, 2, 3}));

!! Thank you for check

EunjuYang · 2024-10-23T01:54:01Z

nntrainer/layers/loss/kld_loss_layer.cpp

+  label.divide_i(predicted);
+  deriv.fill(label);


I think it is just a coding style, but it is just a simple suggestion from my side:

Suggested change

label.divide_i(predicted);

deriv.fill(label);

label.divide(predicted, deriv);

Good Point, i will apply your review

nntrainer/layers/loss/kld_loss_layer.cpp

taos-ci · 2024-10-23T08:57:13Z

cibot: @DonghakPark, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2757-202410231736100.5796000957489-799f131355f83f2286d6282d4c341211a970fac7/.

EunjuYang

LGTM!

taos-ci

@DonghakPark, 💯 All CI checkers are successfully verified. Thanks.

skykongkong8

I think it's good to go!

Currently, there is no unit test for the KLD loss. Therefore, I am adding a new unit test to increase code coverage. - use pre defined unit test - (LayerSemanticsParamType) **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

In the case of NNTrainer, there are many instances where one function works in response to another function, so we have created integration tests for unit testing. - There are exceptional cases depending on the activation when it comes to losses. - For instance with BN (Batch Normalization), you can compare accuracy after conducting several epochs of testing. **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

Implement KLD Loss on NNtrainer **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

Apply coding style with clang-format ( unittest files ) **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

Implement Finalize on KLD Loss **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

Update KLD Loss Function Reflect review **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

taos-ci

@DonghakPark, 💯 All CI checkers are successfully verified. Thanks.

taos-ci

@DonghakPark, 💯 All CI checkers are successfully verified. Thanks.

jijoongmoon

LGTM

djeong20

Great work!

DonghakPark requested review from myungjoo, jijoongmoon, again4you, jaeyun-jung, leemgs, wooksong, helloahn, kparichay, gichan-jang, anyj0527, zhoonit, lhs8928, songgot, jihochu, SeoHyungjun, baek2sm, skykongkong8, djeong20, EunjuYang and a team as code owners October 16, 2024 05:51

github-actions bot added the Need Review label Oct 16, 2024

taos-ci approved these changes Oct 16, 2024

View reviewed changes

SeoHyungjun reviewed Oct 16, 2024

View reviewed changes

EunjuYang reviewed Oct 16, 2024

View reviewed changes

DonghakPark force-pushed the loss_TDD branch 2 times, most recently from 2f24a4e to 3475994 Compare October 16, 2024 23:44

DonghakPark force-pushed the loss_TDD branch from fa092bd to 39fefb4 Compare October 22, 2024 07:50

DonghakPark removed the BUILD/CI label Oct 22, 2024

DonghakPark self-assigned this Oct 22, 2024

taos-ci approved these changes Oct 22, 2024

View reviewed changes

EunjuYang reviewed Oct 23, 2024

View reviewed changes

skykongkong8 reviewed Oct 23, 2024

View reviewed changes

nntrainer/layers/loss/kld_loss_layer.cpp Show resolved Hide resolved

DonghakPark force-pushed the loss_TDD branch 2 times, most recently from b3e0d9a to 799f131 Compare October 23, 2024 08:36

EunjuYang approved these changes Oct 24, 2024

View reviewed changes

DonghakPark force-pushed the loss_TDD branch from 799f131 to d9e182e Compare October 24, 2024 10:31

taos-ci approved these changes Oct 24, 2024

View reviewed changes

skykongkong8 approved these changes Oct 25, 2024

View reviewed changes

DonghakPark added 6 commits October 28, 2024 12:34

[Loss] Implement KLD Loss Layer

1e68631

Implement KLD Loss on NNtrainer **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

[Trivial] apply clang-format for unittest

fe35254

Apply coding style with clang-format ( unittest files ) **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

[Loss] Add finalize to KLD Loss

5704d43

Implement Finalize on KLD Loss **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

[Unnittest & Loss] Update KLD Loss & Fix Unittest

62d65f7

Update KLD Loss Function Reflect review **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghak PARK <[email protected]>

DonghakPark force-pushed the loss_TDD branch from d9e182e to 62d65f7 Compare October 28, 2024 03:35

taos-ci approved these changes Oct 28, 2024

View reviewed changes

DonghakPark changed the title ~~[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/17 17:36~~ [Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 Oct 28, 2024

taos-ci approved these changes Oct 28, 2024

View reviewed changes

jijoongmoon approved these changes Nov 4, 2024

View reviewed changes

DonghakPark added PR/READY2MERGE and removed Need Review labels Nov 5, 2024

djeong20 approved these changes Nov 5, 2024

View reviewed changes

github-actions bot added the Need Review label Nov 5, 2024

DonghakPark removed the Need Review label Nov 6, 2024

jijoongmoon merged commit 27448b6 into nnstreamer:main Nov 7, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 #2757

[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 #2757

DonghakPark commented Oct 16, 2024 •

edited

Loading

taos-ci commented Oct 16, 2024

taos-ci left a comment

SeoHyungjun commented Oct 16, 2024

SeoHyungjun Oct 16, 2024

DonghakPark Oct 16, 2024

SeoHyungjun Oct 16, 2024

SeoHyungjun Oct 16, 2024

DonghakPark Oct 16, 2024

SeoHyungjun Oct 16, 2024

DonghakPark Oct 16, 2024

EunjuYang Oct 16, 2024 •

edited

Loading

DonghakPark Oct 16, 2024

EunjuYang Oct 17, 2024

DonghakPark Oct 17, 2024

taos-ci commented Oct 17, 2024

taos-ci left a comment

EunjuYang left a comment

EunjuYang Oct 23, 2024

DonghakPark Oct 23, 2024

EunjuYang Oct 23, 2024

DonghakPark Oct 23, 2024

taos-ci commented Oct 23, 2024

EunjuYang left a comment

taos-ci left a comment

skykongkong8 left a comment

taos-ci left a comment

taos-ci left a comment

jijoongmoon left a comment

djeong20 left a comment

	label.divide_i(predicted);
	deriv.fill(label);
	label.divide(predicted, deriv);

[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 #2757

[Loss&Test] Implement KLD Loss & Add Unit Test on loss layers @open sesame 10/28 13:48 #2757

Conversation

DonghakPark commented Oct 16, 2024 • edited Loading

In this PR

taos-ci commented Oct 16, 2024

taos-ci left a comment

Choose a reason for hiding this comment

SeoHyungjun commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EunjuYang Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci commented Oct 17, 2024

taos-ci left a comment

Choose a reason for hiding this comment

EunjuYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci commented Oct 23, 2024

EunjuYang left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

skykongkong8 left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

jijoongmoon left a comment

Choose a reason for hiding this comment

djeong20 left a comment

Choose a reason for hiding this comment

DonghakPark commented Oct 16, 2024 •

edited

Loading

EunjuYang Oct 16, 2024 •

edited

Loading