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

Small clean-up in TSpectrum* #151

Closed
wants to merge 6 commits into from
Closed

Small clean-up in TSpectrum* #151

wants to merge 6 commits into from

Conversation

rvernica
Copy link
Contributor

@rvernica rvernica commented Apr 5, 2016

  1. Remove unused SetResolution function and fResolution member from the TSpectrum* classes.
  2. Update and add references to external documentation, such as the *.ps.gz papers and the "manual".

@couet couet self-assigned this Jun 20, 2016
@couet
Copy link
Member

couet commented Jun 27, 2016

Hi,

I see you suppressed all the code related to "fResolution". I agree it might be not used but doing that you changed the calling sequence of some constructors. That will trigger some backward incompatibility. Is it really important for you to clean up it that way ? ... Let us know.

Olivier

@rvernica
Copy link
Contributor Author

I see. I did not realize this might cause issues with backward compatibility.

I did this because of the confusion TSpectrum(Int_t maxpositions, Double_t resolution=1) and SetResolution(Double_t resolution=1) causes. When you read the documentation, you get the impression that you can set the resolution, which is not true. The only way to figure out this is useless it to search for fResolution in the source code.

I can update the patch and leave the function declarations as they are, but make a note in the comments, remove the fResolution member, and empty the body of the SetResolution function. Let me know.

@couet
Copy link
Member

couet commented Jun 27, 2016

One other possibility would be to simply add a comment saying that fResolution is not used right now and could be used/implemented later.

@rvernica
Copy link
Contributor Author

OK. I will do that and update the patch.

@rvernica
Copy link
Contributor Author

I updated the pull request per your comments.

@couet
Copy link
Member

couet commented Jul 20, 2016

Thanks. Update done.

@couet couet closed this Jul 20, 2016
@rvernica
Copy link
Contributor Author

Quick question about the examples in the TSpectrum documentation. Most of them reference the spectra/TSpectrum.root file (example). Is this file available somewhere?

@rvernica
Copy link
Contributor Author

There is a Jira issue about this file https://sft.its.cern.ch/jira/browse/ROOT-6408

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.

3 participants