Add GAMBIT analyses JSON#908
Conversation
GraemeWatt
left a comment
There was a problem hiding this comment.
Thanks for the PR, but let's wait for https://gambitbsm.org/analyses.json to be fixed before merging. The docstring of test_update_analyses should be updated as you did elsewhere. Do you think it's better to link to C++ code in the GitHub repository (example) rather than the relevant analysis in the Gambit documentation (example)?
There was a problem hiding this comment.
Pull Request Overview
This PR adds GAMBIT analysis integration to HEPData, allowing the system to discover and link GAMBIT analysis resources to relevant publication records.
- Adds GAMBIT to the list of supported analysis tools alongside Rivet, MadAnalysis 5, SModelS, CheckMATE, etc.
- Updates user interfaces to include GAMBIT as an option for analysis type
- Adds test coverage for GAMBIT analysis updates
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/records_test.py | Adds test case to verify GAMBIT analysis resource creation |
| hepdata/modules/search/templates/hepdata_search/modals/search_help.html | Adds GAMBIT search example to help documentation |
| hepdata/modules/records/utils/analyses.py | Updates comment to be more generic about analysis tools |
| hepdata/modules/records/templates/hepdata_records/components/resources-widget.html | Adds GAMBIT option to analysis type dropdown |
| hepdata/modules/records/assets/js/hepdata_common.js | Adds GAMBIT file type mapping for UI display |
| hepdata/ext/opensearch/document_enhancers.py | Updates comment to be more generic about analysis tools |
| hepdata/config.py | Adds GAMBIT endpoint configuration |
| hepdata/cli.py | Updates CLI help text to be more generic about endpoints |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@codecov-ai-reviewer review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@codecov-ai-reviewer review |
This comment has been minimized.
This comment has been minimized.
|
Looks like https://gambitbsm.org/analyses.json has been updated for |
mhabedan
left a comment
There was a problem hiding this comment.
Yes, the GAMBIT JSON file should definitely be fixed before merging this PR.
Docstring for test_analyses_update was updated in 5b984f5.
Re. linking C++ code or the GAMBIT documentation: that's a good point, I don't know. It's the one the GAMBIT developers opted for. I'll reach out to them to clarify.
|
Tests are failing because the Combine JSON file from https://cms-public-likelihoods-list.web.cern.ch is inaccessible due to an issue with the CERN GitLab instance (OTG0157579). I guess this is an example of the need for #907. We can re-run the tests when the CERN GitLab instance comes back online. |
|
Related to the issue above with the Combine JSON file being inaccessible, maybe we should log an error in this case (and add an associated test in if response and response.status_code == 200:
[...]
else:
log.error(f'Error accessing {endpoints[analysis_endpoint]["endpoint_url"]}')Currently, there is no |
GraemeWatt
left a comment
There was a problem hiding this comment.
@mhabedan : the Combine JSON file is accessible again and tests are passing. I addressed my last comment in commit 1b9fabf. I was seeing some test failures (example run) due to an exception raised when retrieving the HackAnalysis JSON file. Elsewhere in the HEPData code, we use a function called resilient_requests which implements a retry strategy on top of the normal requests, so I added commit f8dad8f to use it in the update_analyses function. I think this PR is ready to merge now, but let me know if you agree.
|
Hi @GraemeWatt! Thanks for being so fast in addressing these issues before I got the chance! Everything looks fine and ready to merge to me! |
|
Now deployed on hepdata.net, the GAMBIT analyses have been added, and posted on X to advertise. |
Replacement of #886, now based on HEPData branch instead of fork.
#886 (comment) regarding always listing all reinterpretation tools is addressed in 5fe8ac3.
CI will currently fail because GAMBIT analyses JSON is not fully up to latest schema standard, yet. GAMBIT devs are notified so will hopefully be fixed soon.