AEC_exp #397
AEC_exp #397
Conversation
GuyPerets106
requested review from
leondavi,
ohad123,
NoaShapira8 and
Orisadek
as code owners
| @@ -1,5 +1,7 @@ | |||
| #include "nerlWorkerOpenNN.h" | |||
| #include "ae_red.h" | |||
| #include <fstream> | |||
There was a problem hiding this comment.
We don't need fstream and iostream
I guess it was for debug
Please remove
| @@ -28,7 +30,7 @@ namespace nerlnet | |||
| void NerlWorkerOpenNN::perform_training() | |||
| { | |||
| this->_training_strategy_ptr->set_data_set_pointer(this->_data_set.get()); | |||
|
|
|||
| this->_training_strategy_ptr->get_loss_index_pointer()->set_regularization_method(LossIndex::RegularizationMethod::L2); // ! ADDED NOW | |||
There was a problem hiding this comment.
No need. Please validate that this is rebased on top of master.
In master there is already implementation for regularization.
| int num_of_samples = _aec_data_set->dimension(0); | ||
| loss_val_tensor = std::make_shared<fTensor2D>(1, 1); | ||
| (*loss_val_tensor)(0, 0) = static_cast<float>(_last_loss); | ||
| (*loss_val_tensor)(1, 0) = _ae_red_ptr->_ema_event; // Mask the following lines to get reduction in data tranfers sizes, or Unmask to enable AEC stats |
There was a problem hiding this comment.
Add TODO here that we should envelope it with if statement and pass model arg that controls it.
| // Add _aec_all_loss_values to loss_val_tensor | ||
| for (int i = 0; i < num_of_samples; i++) | ||
| { | ||
| (*loss_val_tensor)(3 + i, 0) = (*_aec_all_loss_values)(i, 0); |
There was a problem hiding this comment.
It looks wrong to go through all samples and them one by one.
Add TODO optimization here.
| break; | ||
| *loss_values_return = mse2D; | ||
| _aec_all_loss_values = loss_values_return; | ||
| // cout << "MSE Loss: " << mse_loss << endl; |
| _aec_all_loss_values = loss_values_return; | ||
| // cout << "MSE Loss: " << mse_loss << endl; | ||
| _ae_red_ptr->update_batch(loss_values_mse); | ||
| // cout << "AE_RED RESULT VECTOR:" << endl << *res << endl; |
| *loss_values_mse = mse2D; | ||
| result_ptr = _ae_red_ptr->update_batch(loss_values_mse); | ||
| // string filename = "/tmp/nerlnet/predict_errors.csv"; |
There was a problem hiding this comment.
remove commented code block or move it as a function of ae_red.
| @@ -569,6 +598,11 @@ namespace nerlnet | |||
| } | |||
| curr_layer = curr_layer->get_next_layer_ptr(); | |||
| } | |||
| // Write the model parameters to file | |||
There was a problem hiding this comment.
add a dedicated function that saves model parameters.
We should add this capability to NIF and API-Server at some point.
But remove this comment from here anyway.
src_cpp/opennn
Outdated
There was a problem hiding this comment.
This is maybe the reason for the crash.
We should carefully merge your changes to opennn. I think that Ori did changes as well.
| fTensor2D diff = (*calculate_res - *_aec_data_set); | ||
| fTensor2D squared_diff = diff.pow(2); | ||
| fTensor1D sum_squared_diff = squared_diff.sum(Eigen::array<int, 1>({1})); | ||
| fTensor1D mse1D = (1.0 / static_cast<float>(_aec_data_set->dimension(0))) * sum_squared_diff; | ||
| fTensor2D mse2D = mse1D.reshape(Eigen::array<int, 2>({num_of_samples, 1})); | ||
| fTensor2D mse2D = mse1D.reshape(Eigen::array<int, 2>({(int)num_of_samples, 1})); | ||
| // cout << "MSE2D: " << mse2D << endl; | ||
| *loss_values_mse = mse2D; |
There was a problem hiding this comment.
This can cause a memory issue. You try to put a tensor that will be deleted when this scope ends into a shared pointer. The shared pointer cannot take control of this memory block. This is a wrong usage of shared pointers.
| _ae_red_ptr->update_batch(loss_values_mse); // Update thresholds | ||
|
|
||
| break; | ||
| *loss_values_return = mse2D; |
|
|
||
| break; | ||
| *loss_values_return = mse2D; | ||
| _aec_all_loss_values = loss_values_return; |
There was a problem hiding this comment.
I don't get what the purpose of this line. It is a switch between pointers but at least needs an explanation here.
calculate_sizefunction in nerl_toolsTests: