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

Specify ntransits #43

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

cbrisboi
Copy link
Contributor

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
Copy link
Contributor Author

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

@henrikef
Copy link
Contributor

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 😭

@henrikef henrikef mentioned this pull request Dec 17, 2020
@cbrisboi
Copy link
Contributor Author

cbrisboi commented Dec 17, 2020

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.

hawc_hal/HAL.py Outdated Show resolved Hide resolved
@henrikef
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@cbrisboi
Copy link
Contributor Author

@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.

Copy link
Contributor

@henrikef henrikef left a comment

Choose a reason for hiding this comment

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

Hi @cbrisboi , nice work!

Can you please fix the linter error (see my first comment) so that the tests can run?

I'll review again once that's done. We don't expect the tests on conda to pass right now until the next version of threeML gets tagged, but the tests against the master branch of threeML should pass.

hawc_hal/HAL.py Outdated Show resolved Hide resolved
hawc_hal/maptree/from_hdf5_file.py Outdated Show resolved Hide resolved
Copy link
Contributor

@henrikef henrikef left a comment

Choose a reason for hiding this comment

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

Looks good to me now, only thing left is to get the tests passing.

@henrikef
Copy link
Contributor

henrikef commented Apr 7, 2021

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
Copy link

github-actions bot commented Jul 7, 2021

This PR has become stale. Please check the status

@henrikef
Copy link
Contributor

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

@henrikef
Copy link
Contributor

@xiaojieww Is anyone looking at this?

@xiaojieww
Copy link
Contributor

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot manually specify ntransits
5 participants