fact-project / fact-tools

The fact-tools are an extension to the streams framework to analyse the data of the First G-APD Cherenkov Telescope.
http://sfb876.tu-dortmund.de/FACT/
GNU General Public License v3.0
6 stars 1 forks source link

Parameter improvment and some small new abilities #348

Closed tarrox closed 6 years ago

tarrox commented 6 years ago

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.

maxnoe commented 6 years ago

Please also add -SNAPSHOT to the version in the pom

tarrox commented 6 years ago

Added the -SNAPSHOT to the version.

maxnoe commented 6 years ago

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

maxnoe commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

tarrox commented 6 years ago

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

tarrox commented 6 years ago

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