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

Add pointing interpolated values #223

Merged
merged 8 commits into from
Nov 22, 2019
Merged

Add pointing interpolated values #223

merged 8 commits into from
Nov 22, 2019

Conversation

morcuended
Copy link
Member

Here is the modification to add non-empty gps_time and interpolate the pointing values event-wise from the values contained in the Drive log files.

Something weird happened: I thought I had made a fresh installation of the lstchain repo yesterday, following the instructions written en the README. However, apparently the ctapipe-io-lst package was not pointing to the last version v0.2 as it is specified in environment.yml. So some names within the LSTDriveContainer were not properly set. I had to reinstall it manually to make everything work.

Disclaimer: When adding the pointing interpolated values, the code becomes substantially slower. Maybe this could be optimized. Perhaps it is due to the large file size of the Drive log.

Please have a look and let me know if you have any comment/suggestion

 - Added filling gps_time field with the timestamps taken from UCTS
 - When the UCTS timestamps are not availables take time info from TIB NTP counters
 - This time will be used to interpolate the pointing direction for each event
 - Takes the timing info previously defined
 - The drive log with pointing can be parse as an input optional argument.
 - The pointing info is added only when this file is parsed.
 - Otherwise the only new parameter that dl1 files contain is gps_time taken from UCTS or TIB.
@morcuended
Copy link
Member Author

This is an example of how to run the script adding pointing information:

python lstchain_data_r0_to_dl1.py -f ~/LST-1.1.Run01388.0000.fits.fz \
    -conf config.json \
    -pedestal ~/pedestal_run1575_0000.fits \
    -calib ~/calibration_run1572.hdf5 \
    -pointing ~/drive_log/drive_log_19_09_19.txt

simply parsing the path to the drive log file. The problem at this moment is that one has to check first which drive report file should use because they usually mix several days.

Output:
Screenshot_2019-11-21_14-10-34

Copy link
Member

@vuillaut vuillaut left a comment

Choose a reason for hiding this comment

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

Looks overall good.
I haven't tested with real data.
I am working on getting a real data sample for unit tests, in the meantime, did you run it on real data and get a proper DL1 output ?

ucts_timestamp = event.lst.tel[telescope_id].evt.ucts_timestamp

if ucts_timestamp != 0:
ns = 1e-9 # Convert nanosecs to secs
Copy link
Member

Choose a reason for hiding this comment

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

Using astropy units has been on the todo list for a long time.
Starting now would be great :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

dl1_container.gps_time = gps_time

if pointing_file_path:
pointing_file = pointing_file_path
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@morcuended
Copy link
Member Author

Looks overall good.
I haven't tested with real data.
I am working on getting a real data sample for unit tests, in the meantime, did you run it on real data and get a proper DL1 output ?

Yes, I tested it with one subrun of real data on my laptop I think getting proper output values.

@vuillaut
Copy link
Member

vuillaut commented Nov 21, 2019

@morcuended dl1ab test is failing.
can you see on your laptop why?

@morcuended
Copy link
Member Author

@morcuended dl1ab test is failing.
can you see on your laptop why?

I think it is because that branch was merged in the meanwhile and I did not pull it to my local repo. Will fix it and push again

@morcuended
Copy link
Member Author

morcuended commented Nov 21, 2019

I do not understand why is failing though. The scripts I modified are not dependant on lstchain_mc_dl1ab.py. I pulled the changes on my local repo and ran pytest. It's failing too.

What should I do?

@rlopezcoto
Copy link
Contributor

After pulling the master, did you merge it?
There should be some message of "merged master branch" after you push it and I do not see it.

@morcuended
Copy link
Member Author

After pulling the master, did you merge it?
There should be some message of "merged master branch" after you push it and I do not see it.

Oh, I forgot that last step. I just pushed my branch again.

@morcuended
Copy link
Member Author

Still failing

@rlopezcoto rlopezcoto mentioned this pull request Nov 21, 2019
2 tasks
@rlopezcoto
Copy link
Contributor

Last changes are not merged yet. Can you do:
git fetch upstream
git merge upstream/master
git push

@vuillaut
Copy link
Member

@morcuended weird it's not failing on my system, I'm investigating.

@morcuended
Copy link
Member Author

(dev-lstchain) [dmorcuende@Toshiba cta-lstchain]$ git fetch upstream 
remote: Enumerating objects: 111, done.
remote: Counting objects: 100% (111/111), done.
remote: Compressing objects: 100% (45/45), done.
remote: Total 125 (delta 75), reused 96 (delta 65), pack-reused 14
Receiving objects: 100% (125/125), 20.56 KiB | 0 bytes/s, done.
Resolving deltas: 100% (77/77), completed with 12 local objects.
From https://github.com/cta-observatory/cta-lstchain
 * [new branch]      ctapipe_master -> upstream/ctapipe_master
 * [new branch]      master     -> upstream/master
 * [new branch]      production_scripts -> upstream/production_scripts
 * [new tag]         v0.1.0     -> v0.1.0

(dev-lstchain) [dmorcuende@Toshiba cta-lstchain]$ git merge upstream/master
Already up-to-date.

(dev-lstchain) [dmorcuende@Toshiba cta-lstchain]$ git push
Everything up-to-date

@vuillaut
Copy link
Member

@morcuended actually, master is failing....

@morcuended
Copy link
Member Author

morcuended commented Nov 21, 2019

@morcuended actually, master is failing....

I see. However, when PR #222 was merged, all tests had passed, right?

@vuillaut
Copy link
Member

@morcuended actually, master is failing....

I see. However, when PR #222 was merged, all tests passed, right?

That's why I am a bit confused right now. Working on patch...

@vuillaut
Copy link
Member

#224 should fix the issue.
You'll need to pull master again afterwards.

@rlopezcoto rlopezcoto mentioned this pull request Nov 22, 2019
@rlopezcoto
Copy link
Contributor

@morcuended please merge the new master, after #224 fix tests should pass.

@morcuended
Copy link
Member Author

@morcuended please merge the new master, after #224 fix tests should pass.

Just did it. I think tests are OK now. Thanks a lot, @vuillaut and @rlopezcoto

@rlopezcoto rlopezcoto self-requested a review November 22, 2019 08:15
@rlopezcoto rlopezcoto merged commit e48d9b2 into cta-observatory:master Nov 22, 2019
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