Skip to content

Improve Julia tools packaging and add tests#95

Merged
gipert merged 2 commits intolegend-exp:mainfrom
tdixon97:jl_tests
Mar 19, 2026
Merged

Improve Julia tools packaging and add tests#95
gipert merged 2 commits intolegend-exp:mainfrom
tdixon97:jl_tests

Conversation

@tdixon97
Copy link
Copy Markdown
Collaborator

@tdixon97 tdixon97 commented Feb 7, 2026

  • setup basic infrastructure (real julia package)
  • add CI hooks
  • actually write tests

Copilot AI review requested due to automatic review settings February 7, 2026 20:32
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.13%. Comparing base (befbbc7) to head (047d074).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #95   +/-   ##
=======================================
  Coverage   67.13%   67.13%           
=======================================
  Files          23       23           
  Lines        2054     2054           
=======================================
  Hits         1379     1379           
  Misses        675      675           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR starts setting up a proper Julia package (LegendSimflow) and CI to run Julia tests, and begins moving previously-included helper code into a reusable module.

Changes:

  • Added a Julia package skeleton (LegendSimflow) with Project.toml, module entrypoint, and a Pkg.test() harness.
  • Added a GitHub Actions job to run Julia tests in CI.
  • Refactored the HPGe drift time map script to use the new package instead of including the helper file directly.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
workflow/src/legendsimflow/scripts/Project.toml Adds a Julia project name to the scripts environment.
workflow/julia/LegendSimflow/test/test_lib.jl Introduces a placeholder test file (currently empty).
workflow/julia/LegendSimflow/test/runtests.jl Adds the Julia test entrypoint calling into test_lib.jl.
workflow/julia/LegendSimflow/src/make_hpge_drift_time_maps.jl Switches from include(...)-based helpers to using LegendSimflow.
workflow/julia/LegendSimflow/src/libjl/drift_time_helpers.jl Adds the drift-time helper library code into the package.
workflow/julia/LegendSimflow/src/init-julia-env.jl Adds a Julia environment bootstrap/registry installation helper.
workflow/julia/LegendSimflow/src/LegendSimflow.jl Adds the Julia module entrypoint including the helper library.
workflow/julia/LegendSimflow/Project.toml Defines the new Julia package and dependencies/compat.
.github/workflows/main.yml Adds a CI job to run Pkg.test() for the Julia package.
Comments suppressed due to low confidence (1)

workflow/julia/LegendSimflow/src/make_hpge_drift_time_maps.jl:30

  • using LegendSimflow does not bring compute_drift_time_map (and other helper functions) into scope unless they are exported, and it also no longer implicitly loads dependencies like SolidStateDetectors that this script uses later (e.g., SSD = SolidStateDetectors, Simulation{T}(...)). As written, this script will error with UndefVarError for those names. Either qualify calls (LegendSimflow.compute_drift_time_map, etc.) and add explicit using SolidStateDetectors, or export the intended API from LegendSimflow and import the external deps explicitly here.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread workflow/julia/LegendSimflow/Project.toml Outdated
Comment thread .github/workflows/main.yml Outdated
Comment thread workflow/src/LegendSimflow.jl/test/test_lib.jl
Comment thread workflow/julia/LegendSimflow/src/LegendSimflow.jl Outdated
Comment thread workflow/src/LegendSimflow.jl/src/LegendSimflow.jl
Comment thread workflow/src/LegendSimflow.jl/test/runtests.jl
Comment thread workflow/src/LegendSimflow.jl/Project.toml Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

workflow/julia/LegendSimflow/src/make_hpge_drift_time_maps.jl:30

  • using LegendSimflow doesn’t make the helper functions (e.g., compute_drift_time_map) or external deps (e.g., SolidStateDetectors) available unqualified. The script later calls compute_drift_time_map(...) and references SolidStateDetectors without a module prefix, which will error now that the include("libjl/drift_time_helpers.jl") was removed. Either import the specific names (using LegendSimflow: compute_drift_time_map, ... and using SolidStateDetectors) or update call sites to use LegendSimflow.compute_drift_time_map / LegendSimflow.SolidStateDetectors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

workflow/julia/LegendSimflow/src/make_hpge_drift_time_maps.jl:32

  • make_hpge_drift_time_maps.jl now relies on using LegendSimflow, but this file references SolidStateDetectors (and its types like Simulation, ADLChargeDriftModel, etc.) without importing it anywhere after removing the direct include(...). Also, compute_drift_time_map is called unqualified; that will only work if it’s exported from LegendSimflow (it currently isn’t). Add the missing using SolidStateDetectors (and any other needed deps) here, and either qualify the helper call as LegendSimflow.compute_drift_time_map(...) or export/import that symbol from the package.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread workflow/src/legendsimflow/scripts/Project.toml Outdated
Comment thread workflow/src/legendsimflow/scripts/Project.toml
Comment thread workflow/src/LegendSimflow.jl/src/drift_time_helpers.jl
Comment thread workflow/src/legendsimflow/scripts/make_hpge_drift_time_maps.jl
@gipert
Copy link
Copy Markdown
Member

gipert commented Feb 8, 2026

also missing: update snakemake rules

@tdixon97 tdixon97 marked this pull request as draft February 8, 2026 11:38
@tdixon97
Copy link
Copy Markdown
Collaborator Author

tdixon97 commented Feb 8, 2026

Yes there is still quite a lot to do.

@gipert
Copy link
Copy Markdown
Member

gipert commented Feb 10, 2026

Please also add some constraints to the julia package versions in the Project.toml file.

@tdixon97
Copy link
Copy Markdown
Collaborator Author

@gipert I think we should finish and merge this structural change (tomorrow?) and then add more tests and details later. I am not so sure what changes you want to Project.toml
@aider-chat-bot do you know what he means?

@gipert
Copy link
Copy Markdown
Member

gipert commented Feb 12, 2026

It's very easy and also important to do asap

https://pkgdocs.julialang.org/v1/compatibility/

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

workflow/julia/LegendSimflow/src/make_hpge_drift_time_maps.jl:35

  • SolidStateDetectors is referenced later in this script (e.g., Simulation{T}, SolidStateDetector, ADLChargeDriftModel), but after switching from include(...) to using LegendSimflow it is no longer imported into this file scope. Add an explicit using SolidStateDetectors (or fully-qualify usages) so the script doesn’t fail with UndefVarError: SolidStateDetectors not defined.
    workflow/julia/LegendSimflow/src/libjl/drift_time_helpers.jl:268
  • The compute_drift_time_map docstring still documents the old signature/order (compute_drift_time_map(sim, meta, T, angle_deg, grid_step)) and argument list, but the function now takes (sim, meta, angle_deg, grid_step, padding, T). Update the docstring signature and the # Arguments section so callers don’t use the wrong parameter order.
    workflow/julia/LegendSimflow/src/libjl/drift_time_helpers.jl:268
  • Changing compute_drift_time_map’s positional argument order is a breaking API change and currently conflicts with the existing call site in make_hpge_drift_time_maps.jl (which passes T as the 3rd argument). Consider restoring the original signature (or adding a compatibility wrapper method) so existing scripts keep working.
    workflow/julia/LegendSimflow/src/libjl/drift_time_helpers.jl:278
  • build_simulation_grid_axis requires boundary::T, grid_step::T where T<:AbstractFloat, but here radius/height are computed as default Float64 and grid_step is only constrained to Real. This will throw a MethodError if grid_step is an Int or a different float type than radius/height. Convert radius, height, and grid_step to T before calling, or tighten the compute_drift_time_map signature to accept grid_step::T/boundary::T.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread workflow/src/LegendSimflow.jl/test/test_lib.jl
@tdixon97 tdixon97 marked this pull request as ready for review March 18, 2026 22:29
@gipert gipert changed the title Add tests for the julia code Improve Julia tools packaging and add tests Mar 19, 2026
gipert and others added 2 commits March 19, 2026 10:35
Promotes the ad-hoc Julia scripts in legendsimflow/scripts/ to a proper
Julia package at workflow/src/LegendSimflow.jl, with a Project.toml,
versioned source code and tests. Updates Snakemake rules to reference
the new package location and adds a CI job to run the Julia tests.

Co-Authored-By: Toby Dixon <toby.dixon.23@ucl.ac.uk>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Toby Dixon <toby.dixon.23@ucl.ac.uk>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gipert gipert merged commit d7a810d into legend-exp:main Mar 19, 2026
10 checks passed
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