Specify ntransits

[cbrisboi]

This pull request fixes #32.

May not be sufficiently tested (yet), but I believe the essential functionality is there, so I opened the request.

This allows for subsets of data to be properly analyzed which may only contain (for example) 300 days of data taken over 1500 days. This allows the user to set the HAL plugin to compute the flux over the full 1500 days rather than the 300 days of data contained in the map itself.

[cbrisboi]

I guess this causes a few of the tests to fail. I'll take a look this afternoon

[henrikef]

Good news for you: You can ignore the issues with python 2, and the errors in the python3 tests might be unrelated to your changes. I just opened a pull request to remove the python2 tests and I'm seeing the same errors opening ROOT files 😭

[cbrisboi]

oh noes, maybe we need to work on the root file reading dependencies next. That is good news for me though I will take another look to make sure. Thanks!

I have at least one other functional (rather than test related) item I want to check, I want to confirm if LiFF uses sidereal or solar days in its conversion from the hours in the file to ntransits. HAL appears to use 24 hours/transit, and if liff is different I wanted to make that consistent. Its a small difference, but I think consistency would be better than not.

[henrikef]

Yes, #45 needs to be fixed before this can be merged. Maybe it's an easy fix to the interface, maybe we can fix the version of root to be installed. I'm not sure when I'll be able to work on it though.

The difference between sidereal and solar days is way below our systematic uncertainty, so I wouldn't spend too much time hunting it down at this stage (maybe add a separate issue to come back to later).

Can you please add a simple unit test for this new feature? Doesn't have to be complex, just to show the value of n_transits is set correctly in either use case (user provided value or read from file).

[henrikef]

Any updates on the unit tests for this pull request @cbrisboi ? The issue with the TFile interface has been fixed on the master branch.

[maloneka]

If I am understanding correctly, this can be merged as soon as the unit test is written. The issue mentioned in #45 was fixed in #48, which has been merged.

[henrikef]

Yes, I think so. @cbrisboi might have to merge the current master branch into this one to get all the tests to run though. Independently of adding a unit test, it would also be good to see if this actually gives reasonable results on something like the Crab strip.

[henrikef]

@cbrisboi any updates on this? Will you add some unit tests so we can merge this?

[cbrisboi]

@henrikef I have merged threeML/hawc_hal master branch into this branch and added a unit test. I found during the process of writing the unit test that the hdf5 and root files transit fields were being treated differently, I have fixed that now that it was identified from failing the test.

[henrikef]

Hi @cbrisboi!

Thanks for implementing this!

Please pull in the most recent version of the hawc_hal master into your branch and resolve any conflicts. Note that the tests directory moved to hawc_hal/tests.

Once the tests pass, this is ready to merge.

[github-actions[bot]]

This PR has become stale. Please check the status

[henrikef]

@cbrisboi has since left the collaboration. Any volunteers to finish this (adding tests) so it can be merged?

[henrikef]

@xiaojieww Is anyone looking at this?

[xiaojieww]

@xiaojieww Is anyone looking at this?

Seems not currently. I plan to solve the #68 first, if still no one working on this I will take a look at it.

[torresramiro350]

I think this request can be closed since #84 has been merged.