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

Added exponential shaping function to TRestRawSignalShapingProcess #58

Merged
merged 6 commits into from
May 9, 2022

Conversation

DavidDiezIb
Copy link
Member

@DavidDiezIb DavidDiezIb commented May 6, 2022

DavidDiezIb Ok: 26

New response fuction added to TRestRawSignalShapingProcess, the exponential function: exp(-x/t)

} else if (fShapingType == "exponential") {
Nr = (Int_t)(5 * fShapingTime);

rsp = new double[Nr];
Copy link
Member

Choose a reason for hiding this comment

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

We should in general avoid to use new, particularly in standard data types such as double. I think we can easily replace by std::vector<double> in this case.
I know this issue was not introduced in this PR, but we should try to fix it, let me know in case of doubts, I can provide code changes.

@DavidDiezIb
Copy link
Member Author

DavidDiezIb commented May 6, 2022

@juanangp Not sure what's is the issue with new, @lobis has encourage me in same direction but I think don't undestand the problem/risk.
Do you prefer the approach of this last commit? I think we loss all the advantages of using a pointer. Or you mean removing completely the pointer and use directly a std::vector<double>?

@lobis
Copy link
Member

lobis commented May 6, 2022

@juanangp Not sure what's is the issue with new, @lobis has encourage me in same direction but I think don't undestand the problem/risk. Do you prefer the approach of this last commit? I think we loss all the advantages of using a pointer. Or you mean removing completely the pointer and use directly a std::vector<double>?

We should only use pointers whenever it is requiered (which is not that common), and in this cases we should attempt to use smart pointers (https://en.cppreference.com/book/intro/smart_pointers), root may give issues with this so it may not be always possible.

In this case I don't think using pointers serves any purpose, but I have not reviewed the code in detail.

Also we should not use C style arrays either (stuff like double[n] array etc.) and should only use STL containers such as std::vector (https://stackoverflow.com/questions/7129717/what-does-c-style-array-mean-and-how-does-it-differ-from-stdarray-c-style).

Also in my opinion we should always try to replace old variable names with more explicit variable names, i.e. rsp -> response.

It is also not necessary (and not recommended IMO) to use std:: in the source files for things such as vector (I saw your last commit introduced this change), since all sources are using the std namespace.

@juanangp
Copy link
Member

juanangp commented May 6, 2022

@juanangp Not sure what's is the issue with new, @lobis has encourage me in same direction but I think don't undestand the problem/risk. Do you prefer the approach of this last commit? I think we loss all the advantages of using a pointer. Or you mean removing completely the pointer and use directly a std::vector<double>?

While using new and delete you are allocating and deallocating memory in the heap which is less efficient and more error prone than allocating memory in the stack. Check the following link for more details https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new

The solution that you provide using c style arrays is fine to me since you are not recursivelly calling new and delete anymore. I was proposing a c++ solution using std::vector but I have nothing against this implementation.

Perhaps you should remove the delete towards the end of the function, I will provide a commit suggestion about this.

@lobis lobis self-requested a review May 6, 2022 12:12
@juanangp
Copy link
Member

juanangp commented May 6, 2022

As a general comment, the pipeline was failing, it seems that your local branch was quite behind master, please make sure that your branch is up-to-date before creating the PR. To merge master with your local branch you just have to do git pull origin master, note that you can do at any time, although perhaps you need to resolve conflicts if any.

I have changed a little the code, using std::vector::resize because there is no need to define the double *. I will be happy to approve once pipeline succeed.

@DavidDiezIb DavidDiezIb merged commit c8aa741 into master May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants