Parameter improvment and some small new abilities #348

tarrox · 2018-03-16T11:38:40Z

Improvement to the current processors by adding additional parameters to set,
primarily adding the ability to set the UnixTimeUTC key.
Added the ability to reverse the process for the DrsCalibration and the remapping.
Added a processor to combine two arrays into one by combining each element, given the four std operators.

This changes are necessary for the work I do, to make the pull easier I split the last pull request #246 up into three pull request this being the first one.

…elkey

…vision

maxnoe

Some unneeded getters and setters

Why are "UnixTimeUTC" and "StartCellData" Parameters now? I think this is not needed and only adds fuzz.

maxnoe · 2018-03-16T11:44:04Z

src/main/java/fact/datacorrection/InterpolateTimeSeries.java

+    public String badPixelKey = "badPixels";
+
+    @Parameter(required = false, description = "The key to the unixTimeUTC of the Event.")
+    public String unixTimeKey = "UnixTimeUTC";


Why is this a parameter?

I use this processor twice for different inputs with different times one will use 'UnixTimeUTC' the other 'LONS_UnixTimeUTC'. This is due to the fact that i have to calibrate the pedestal that I superimpose.

maxnoe · 2018-03-16T11:44:17Z

src/main/java/fact/datacorrection/InterpolateTimeSeries.java

@@ -36,6 +36,12 @@
    @Parameter(required = false, description = "The minimum number of neighboring pixels required for interpolation", defaultValue = "3")
    public int minPixelToInterpolate = 3;

+    @Parameter(required = false, description = "The key for the resulting badPixelSet.")
+    public String badPixelKey = "badPixels";


Why is this an xml Parameter?

I use the processor twice, so i added this in case someone want to preserve the output and not override the key with the second processor. I think it is cleaner this way, but I can remove it.

maxnoe · 2018-03-16T11:44:29Z

src/main/java/fact/datacorrection/PatchJumpRemoval.java

@@ -38,6 +38,9 @@
    @Parameter(required = true)
    public String startCellKey = null;

+    @Parameter(required = false, description = "The key containing the UnixTimeUTC")
+    public String unixTimeKey = "UnixTimeUTC";


Why is this a parameter?

I use this processor twice for different inputs with different times one will use 'UnixTimeUTC' the other 'LONS_UnixTimeUTC'. This is due to the fact that i have to calibrate the pedestal that I superimpose.

maxnoe · 2018-03-16T11:44:37Z

src/main/java/fact/features/source/SourcePosition.java

@@ -58,6 +58,9 @@
    @Service(required = false, description = "Name of the service that provides aux files")
    public AuxiliaryService auxService;

+    @Parameter(required = false, description = "The key containing the UnixTimeUTC")
+    public String unixTimeKey = "UnixTimeUTC";


Why is this a Parameter?

I use this processor twice for different inputs with different times one will use 'UnixTimeUTC' the other 'LONS_UnixTimeUTC'. This is due to the fact that i have to get the sourceposition of the pedestal that I superimpose.

What is LONS supposed to mean?

'Light of night sky' before it was PED but we already have stuff that is called pedestal so I use LONS, that way there is no conflict with the other stuff.

I'm against inventing new abbreviations that nobody uses. The commonly used abbreviation is NSB for night sky background

maxnoe · 2018-03-16T11:45:24Z

src/main/java/fact/utils/CombineDataArrays.java

+    }
+
+
+    public static Logger getLog() {


maxnoe · 2018-03-16T11:45:29Z

src/main/java/fact/utils/CombineDataArrays.java

+    }
+
+
+    public static void setLog(Logger log) {


maxnoe · 2018-03-16T11:45:40Z

src/main/java/fact/utils/PreviousEventInfo.java

@@ -18,7 +18,11 @@
    @Parameter(required = true)
    public String outputKey = null;

-    int limitEvents = 20;
+    @Parameter(required = false, description = "The key containing the UnixTimeUTC")


Why is this a parameter?

For the case someone wants to set it, we have an variable in 'settings.properties' for this but no way to set it in this processor (only in the jumpRemoval). So either this one goes or, the thing in setting.properties makes little sense because it is a constant.

maxnoe · 2018-03-16T11:47:51Z

Please also add -SNAPSHOT to the version in the pom

tarrox · 2018-03-16T12:29:39Z

Added the -SNAPSHOT to the version.

maxnoe · 2018-03-16T13:16:51Z

As 1.0.1 is released now, the version for this should be 1.0.2-SNAPSHOT

jebuss

Nice work! The UnixTimeKey stuff is ok for me. Couldyou maybe provide an example XML that is using the TimeseriesFeatures and the CombineDataArrays? If you put that to examples/studies it will run with the unit tests (?) and one can see how and why it is used. Otherwise, I would asume it would be deleted in later versions.

maxnoe · 2018-04-28T13:44:53Z

As we now require the timestamp key anyways, we could remove conversions from unixtimeutc to zoneddatetime and just do it once, maybe even in the readers and just use the timestamp key everywhere

jebuss · 2018-04-30T09:33:24Z

As we now require the timestamp key anyways, we could remove conversions from unixtimeutc to zoneddatetime and just do it once, maybe even in the readers and just use the timestamp key everywhere

I'm not sure if I know what you mean. Could you point me to a file / line?

tarrox · 2018-05-02T11:07:06Z

I'm not sure if I know what you mean. Could you point me to a file / line?

I think he means this neat feature here <fact.features.UnixTimeUTC2DateTime /> which does the conversation once at the beginning. This would remove the necessity to convert it be hand in every processor like in datacorrection/InterpolateTimeSeries.java:52.
I would agree with Max point that this would indeed be better but I would suggest to leave it at this for now and create a different branch where every processor gets updated to the timestamp version.

maxnoe · 2018-05-02T13:53:50Z

I'm not sure if I know what you mean. Could you point me to a file / line?

@jebuss Everywhere we do calculations involving timestamps we use ZonedDateTime. We always convert UnixTimeUTC into timestamp. We could just do this once, maybe even in the readers instead of doing it in each processor that needs to do stuff with a timestamp.

We use Utils.unixTimeUTCToZonedDatetime in these processors

src/main/java/fact/cleaning/TwoLevelTimeMedian.java
src/main/java/fact/cleaning/TwoLevelTimeNeighbor.java
src/main/java/fact/features/UnixTimeUTC2DateTime.java
src/main/java/fact/features/source/CameraToEquatorial.java
src/main/java/fact/features/source/SourcePosition.java
src/main/java/fact/datacorrection/InterpolateTimeSeries.java
src/main/java/fact/datacorrection/InterpolatePixelArray.java
src/main/java/fact/extraction/BasicExtraction.java
src/test/java/fact/io/hdureader/BinTableTests.java

jebuss · 2018-05-16T09:50:30Z

Ok, @maxnoe I see your point. IMHO, it is neither a show stopper for this PR nor is there a larger clarity in opening a new issue and a new PR for this, since this PR is anyways a collection of several features. So both ways do make sense.

So I would say its up to @tarrox if he can put afford in adding the timestamp in the readers and adapt the according processors for this PR. It does not seem to be much of an afford. However, If he does not have time right now, I would also vote for opening an issue and and a new PR for this feature.

I would be happy if we could finish this PR soon.

maxnoe · 2018-05-16T15:05:54Z

src/main/java/fact/datacorrection/DrsCalibration.java

+            data.put(outputKey, calibrated);
+        else {
+            short[] decalibratedShortData = new short[calibrated.length];
+            // System.arraycopy(rawData, 0, rawfloatData, 0, rawfloatData.length);


Commented code

Removed this one and the other.

maxnoe · 2018-05-16T15:06:23Z

src/main/java/fact/datacorrection/DrsCalibration.java


-        // add color value if set
+        //TODO add color value if set


Please just remove this line

maxnoe · 2018-05-16T15:06:50Z

src/main/java/fact/datacorrection/DrsCalibration.java

-                vraw /= drsGainMean[offsetPos];
-                vraw *= 1907.35;
+                triggerOffsetPos = pixel * drsTriggerOffsetMean.length / Constants.N_PIXELS
+                        + slice;


this line break makes things worse imho

fixed this one and the other ones too

maxnoe · 2018-05-16T15:08:24Z

src/main/java/fact/datacorrection/DrsCalibration.java


-        // add color value if set
+        //TODO add color value if set

        return data;
    }

    public double[] applyDrsCalibration(double[] data, double[] destination,


I still think two functions would be more clear. applyDrsCalibration and revertDrsCalibration.

split into two functions

tarrox · 2018-05-16T20:49:29Z

Replaced the UnixTimeUTC with timestamp in the processors where it makes sense.

maxnoe · 2018-05-17T07:48:20Z

src/main/java/fact/datacorrection/DrsCalibration.java

+     * @return The decalibrated data array
+     */
+    public double[] reverseDrsCalibration(double[] data, double[] destination, short[] startCellVector) {
+        if (destination == null || destination.length != data.length)


Maybe throwing in case the length do not match is better, what do you think?

This was legacy code, but yeah i can't think of any reason where giving a different length is intentional and not an error or lazyness. Changed in the next commit.

maxnoe · 2018-05-17T07:49:15Z

src/main/java/fact/datacorrection/PatchJumpRemoval.java


        // Get variables out of data item
-        int[] currentTime = (int[]) item.get("UnixTimeUTC");


This processor apperently still uses UnixTimeUTC and not timestamp

This is currently intentional, this is because the previousEventInfo holds UnixTimeUTC and not timestamp, this would need to be changed. I would suggest its own PR, especially as the SamplePedestalEvent processor needs also to be changed (currently in its own branch, PR will be created once this PR is finished).

…t/fact-tools into parameter_improvment

tarrox · 2018-05-18T11:06:44Z

Added issue #362 for the missing unixtime->timestamp changes. So we can finish this PR quickly.

tarrox added 8 commits March 16, 2018 10:06

improved drscalibration adding more parameters and reverse option

53df3e5

added unixtimekey to the processors and the ability to set the badpix…

d351275

…elkey

added reversing to previousEventInfo and imroved the parameters

b1dfd9c

Added the ability to substract smoothed data from the timeseries

7e2d3db

Added the ability to substract the movingAvg from the timeseries

4c9ce3e

New processor for combining two arrays, with either add,sub,mul or di…

e8acd32

…vision

exchanged tabs for spaces

4259f89

fixed some privates into public

1bf9e96

tarrox requested review from kbruegge and maxnoe March 16, 2018 11:38

maxnoe requested changes Mar 16, 2018

View reviewed changes

tarrox added 2 commits March 16, 2018 13:23

removed not needed getter/setter

1a75fcf

changed to snapshot

25855be

tarrox mentioned this pull request Mar 16, 2018

Hdu reader skip rows #349

Merged

small conflict with the version

999a5bc

fixed the version to the next one in line

341420e

jebuss approved these changes Apr 27, 2018

View reviewed changes

maxnoe requested changes May 16, 2018

View reviewed changes

tarrox added 3 commits May 16, 2018 21:46

split drsCalibration into two functions

8bcc014

switched from UnixTimeUTC to timestamp

73656ff

merged with master

ce6c178

Merge branch 'master' into parameter_improvment

8658e57

maxnoe reviewed May 17, 2018

View reviewed changes

tarrox added 2 commits May 17, 2018 20:27

unequal array is now an error within the calibration functions

92c35ca

Merge branch 'parameter_improvment' of https://github.com/fact-projec…

69a8e69

…t/fact-tools into parameter_improvment

maxnoe approved these changes May 18, 2018

View reviewed changes

jebuss merged commit ac9d7a4 into master May 18, 2018

jebuss deleted the parameter_improvment branch May 18, 2018 15:28


		// Get variables out of data item
		int[] currentTime = (int[]) item.get("UnixTimeUTC");

Parameter improvment and some small new abilities #348

Parameter improvment and some small new abilities #348

Conversation

tarrox commented Mar 16, 2018

maxnoe 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

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

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

maxnoe commented Mar 16, 2018

tarrox commented Mar 16, 2018

maxnoe commented Mar 16, 2018

jebuss left a comment

Choose a reason for hiding this comment

maxnoe commented Apr 28, 2018

jebuss commented Apr 30, 2018

tarrox commented May 2, 2018

maxnoe commented May 2, 2018

jebuss commented May 16, 2018

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

Choose a reason for hiding this comment

tarrox commented May 16, 2018

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

tarrox commented May 18, 2018