search

threeML / hawc_hal

HAWC Accelerated Likelihood - python-only framework for HAWC data analysis
BSD 3-Clause "New" or "Revised" License
11 stars 21 forks source link

Hal uproot #78

Closed torresramiro350 closed 2 years ago

torresramiro350 commented 2 years ago

@xiaojieww HAL now has no dependencies on ROOT or root-numpy when opening maptrees and detector response files in the ROOT format. Previous functionality has been adapted using an alternative library uproot. There are some packages that are necessary for plotting and obtaining information from histograms: mplhep, and hist and can be installed via pip. Tests cases pass on the Linux Fedora 36 side, but cannot test directly on Mac.

henrikef commented 2 years ago

Hi Ramiro! If additional packages are required, could you please add them to setup.py ?

torresramiro350 commented 2 years ago

I've added the required packages into setup.py

xiaojieww commented 2 years ago

Tested both on macOS Monterey chip M1 and M1 Pro, installation and pytest --pyargs hawc_hal and pytest --pyargs astromodels are succeed, but the test for threeML pytest --pyargs threeML failed.

Failure information see below. They are all AttributeError: 'timeout'. test_threeML_error.log

xiaojieww commented 2 years ago

@torresramiro350 Seems there are some conflicts between this branch and the master one, could you help solve this? Also, could you run the pytest --pyargs threeML for the current master branch to confirm if this still has failures? I tested on my cluster, it shows failures. I haven't received more message from our 3ML colleagues, but @henrikef think the failures for pytest --pyargs threeML is a known issue, and the particular test should have been disabled. If it the case, @henrikef do you think should we ignore this and just go ahead and merge it? (Since the test on both M1 and M1 pro chips worked)

torresramiro350 commented 2 years ago

Conflicts are because of the merge in the master branch with pull request #80. It seems to resolve the conflict, I need to open another request.

henrikef commented 2 years ago

@torresramiro350 You can merge the current master branch into your fork, resolve the conflicts locally, then push.

henrikef commented 2 years ago

@xiaojieww Something is very wrong with the github actions, if you click on "checks" for this pull request is says no jobs were run. I am not in a position to trouble shoot this right now, please reach out to other HAL users within HAWC for that.

In the end, it is your decision when to merge. I would however highly recommend that you make sure the tests actually run and pass. Also, that tests are added for any new features included in each pull request - for example, this pull request also includes the "radial profile" feature from #68. Generally, it is cleaner to keep one new feature per pull request.

If you don't hear back from the threeML folks, maybe ask them again - sometimes things get lost.

jasonfan1997 commented 2 years ago

Sorry to jump into the discussion. @xiaojieww I failed to install it on python 3.7 for some reason, but I can run all the tests on python 3.8 at fiesta. Also, it seems like the error you got is related to the network instead of the code, did you test on another network or another machine?

xiaojieww commented 2 years ago

Sorry to jump into the discussion. @xiaojieww I failed to install it on python 3.7 for some reason, but I can run all the tests on python 3.8 at fiesta. Also, it seems like the error you got is related to the network instead of the code, did you test on another network or another machine?

No worries, thanks for the testing and information! I tried both on the cluster and my laptop. Both installations succeed. For the testings, all the pytest passed on the cluster, and only the 3ml test pytest --pyargs threeML failed for the M1 chip. So I'm wondering it's not because of the internet issue. If I didn't remember wrong, using M1 Pro chips also has the same issue.

xiaojieww commented 2 years ago

@torresramiro350 Since the tests only failed on Mac, I planned to merge your request shortly. I updated your build_and_test.yml and removed the root_numpy related lines. If the tests pass, I think it should be good to go now.

jasonfan1997 commented 2 years ago

@xiaojieww again to jump into the discussion. I finally remember to try to install hal on a mac M1 at my office and pytest --pyargs threeML passed the test so I think the branch should be fine. Had you heard back from the 3ML team?

xiaojieww commented 2 years ago

@jasonfan1997 I didn't hear back. I also tested on my M1 chip laptop yesterday, still, only the pytest --pyargs threeML failed with the same error I listed above. How about the other two pytest you did on your laptop? Would you mind tested again and let us know the software version you are using? I'm using macOS Monterey version 12.0.1 for mine.

jasonfan1997 commented 2 years ago

@xiaojieww They both passed. I am still using Big Sur 11.6 on my mac mini so that might be the difference. I will test it again after updating to Monterey

jasonfan1997 commented 2 years ago

@xiaojieww I upgraded to Monterey and reinstalled everything, but the test still passed. Could you verify that running pytest --pyargs threeML without hawc_hal installed and with hawc_hal master branch installed passes the test?

xiaojieww commented 2 years ago

@xiaojieww I upgraded to Monterey and reinstalled everything, but the test still passed. Could you verify that running pytest --pyargs threeML without hawc_hal installed and with hawc_hal master branch installed passes the test?

Weird. For master branch, I can't install root_numpy on Mac successfully. Without install the hal, still can't pass the pytest --pyargs threeML, but pytest --pyargs astromodels is fine.

xiaojieww commented 2 years ago

Thanks @jasonfan1997 for help with the testing on Mac. All the pytest are passed on his M1 chip laptop. @torresramiro350 Also tested on his laptop, and all the tests passed. Same for my other tests on the cluster.

The previous failure may be caused by a local internet setting as issue #315. I tested python -c "from threeML.io.network import internet_connection_is_active; print (internet_connection_is_active())", it give back timeout error for me as below, while a true for @jasonfan1997 

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/xwang32/hawc_software/miniconda3/envs/new_hal_pr68/lib/python3.9/site-packages/threeML/io/network.py", line 33, in internet_connection_is_active
    print(ex.message)
AttributeError: 'timeout' object has no attribute 'message'

Although the CI test is still failing for mac, I think this pull request is good to go. So I will merge it.