-
Notifications
You must be signed in to change notification settings - Fork 1
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
Single pulse extraction improvement #164
Conversation
This reverts commit 7427bba.
This reverts commit 0b05075.
This reverts commit e70b581.
This reverts commit 9ad81c0.
This reverts commit 3be48d9.
This reverts commit 7427bba.
This reverts commit ef23565.
…alized each subtraction time, since we do not forsee it to change in amplitude.
…um once from the timeseries in the beginning of the extraction.
…xtractor Non static API for single pulse extractor.
Bumb version number. Going straight to 0.17 because we introduce java8.
Bump version number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me. I made some comments on some minor things. Can be merged now howerver. In my opinion.
* @param array2d 2dim double array | ||
* @return flattend double array | ||
*/ | ||
public static double[] flatten2dArray(double[][] array2d) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure we need an extra function for this. The one line you wrote here is pretty nice and already right? Maybe it can be used directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to keep this for now.
|
||
public Config config; | ||
|
||
public double[] pulseToLookFor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but it looks to me like some of these variables could be made final. Just to add a bit more rigor to the class. So nobody gets the idea to change these references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot set them final
import stream.Processor; | ||
import stream.annotations.Parameter; | ||
|
||
import javax.rmi.CORBA.Util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this is. Probably an accidental import right? sometimes intellij is too fast for me ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I also think that this can be merged. If it is not superduper urgent I would suggest to have look on friday and then merge it. Maybe we could even fix #162 by then. |
…/fact-project/fact-tools into singlePulseExtractionImprovement
delete maxIterations
delete maxIterations
delete maxIterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The API is much more flexible now (no more static global variables). I would like to make this a production release (in the master) as soon as possible to process all our data at ISDC before it is only available on tapes.