Skip to content

Conversation

@ggalloni
Copy link
Collaborator

@ggalloni ggalloni commented Jan 7, 2026

This PR attempts adding ty to perform automatic type-checking.

Right now, this the situation in terms of errors detected by ty [rule: number of violations] (-> remaining after edit):

  • not-subscriptable: 537
  • invalid-argument-type: 309
  • invalid-assignment: 256
  • possibly-missing-attribute: 167
  • unresolved-attribute: 102
  • unsupported-operator: 60
  • invalid-parameter-default: 30
  • not-iterable: 25
  • unresolved-import: 16
  • no-matching-overload: 12
  • invalid-return-type: 11
  • unresolved-reference: 10
  • missing-argument: 8
  • parameter-already-assigned: 5
  • deprecated: 4
  • invalid-type-arguments: 4
  • unknown-argument: 3

Thus, this will require some time.

@ggalloni ggalloni added bug Something isn't working enhancement New feature or request labels Jan 7, 2026
@github-actions
Copy link

github-actions bot commented Jan 7, 2026

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  litebird_sim
  bandpass_template_module.py 133-139
  bandpasses.py 246-247
  beam_convolution.py 319, 406-408, 416-418
  detectors.py
  dipole.py 139-175
  gaindrifts.py 392-396, 450
  grasp2alm.py
  hwp_diff_emiss.py 20-21, 42-44
  hwp_harmonics.py 37, 41-44, 300-303, 403-406
  imobrowser.py 83, 93-96, 115-118, 121-124, 127-131, 134-138, 144-147, 163-165
  io.py 478, 481, 806
  madam.py
  observations.py 356, 440-445, 770-789
  plot_fp.py 32-38, 129, 143-148, 193-197, 200, 229-233
  pointing_sys.py 483-485, 555, 562-573
  pointings.py 212, 215-218
  pointings_in_obs.py 173
  scan_map.py 56-57, 274-280, 423-425
  scanning.py 210-213, 325-326, 490
  simulations.py 228-230, 436, 439, 489, 693, 828, 937, 1270, 1315-1316, 1565-1566, 1685-1686, 1871, 1880, 2100-2106, 2222-2228, 2268-2273, 2529-2534
  spacecraft.py 302
  litebird_sim/imo
  imo.py 38-46, 58-64
  litebird_sim/mapmaking
  binner.py 176, 265
  destriper.py 112, 417-419, 766-767, 995, 1140, 1792-1793
  litebird_sim/mbs
  mbs.py 69-70, 977-982
Project Total  

The report is truncated to 25 files out of 37. To see the full report, please visit the workflow summary page.

This report was generated by python-coverage-comment-action

@ggalloni ggalloni marked this pull request as ready for review January 9, 2026 08:23
@ggalloni
Copy link
Collaborator Author

ggalloni commented Jan 9, 2026

@ziotom78 @paganol I think this is ready for review.
All the issues reported above are solved, except unresolved-import. Indeed I decided to just ignore that since we will always have some unresolved stuff depending on which optional dependencies one has installed for example. Anyway, it is not a strong source of bugs I would say.

Types are checked both at CI level within the code-style check and I added a pre-commit hook to check them at commit.
I feel like we should keep an eye on this second one 'cause it might be be too strict. What do you think?

Along the way, the checker highlighted real sources of bugs that weren't caught by tests (great!), as missing methods, wrong types with wrong operators, and many others. Also, I think it will be extremely useful to track architectural changes as the new SphericalHarmonics, since the change in type of maps is propagated on the full repo and tells you where it breaks!

A limitation seems to be that numba doesn't expose the correct types somehow, so stuff like prange is not properly recognized and I had to ignore it.

Note that I excluded from type checks these folders:

  • "docs/"
  • "benchmarks/"
  • "test/"
  • "scripts/"
  • "litebird_sim/mbs/" (this was quite messy, but since it is going to disappear I didn't bother fixing it)

Let me know what you think of this.

Copy link

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 pull request adds the ty type checker (version 0.0.9) to the project to perform automatic type-checking. The PR includes configuration for ty in pyproject.toml, integration with pre-commit hooks and CI workflows, and numerous type annotation improvements and bug fixes across the codebase.

Key changes:

  • Addition of ty as a development dependency with appropriate configuration
  • Type annotation improvements using modern Python syntax (e.g., int | None instead of Optional[int])
  • Fixes for type-related issues including proper use of assertions and getattr patterns
  • Bug fixes discovered during type checking (typo in allow_pickle, incorrect variable reference in MBS)

Reviewed changes

Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
uv.lock Added ty 0.0.9 dependency with platform-specific wheels
pyproject.toml Added ty configuration and development dependency
.pre-commit-config.yaml Added pre-commit hook for ty type checking
.github/workflows/codestyle.yml Added CI check for ty
litebird_sim/simulations.py Extensive type annotations, getattr usage, assertions
litebird_sim/observations.py Dynamic attribute declarations, type annotations
litebird_sim/spherical_harmonics.py Union type syntax improvements
litebird_sim/spacecraft.py Type annotations and assertions
litebird_sim/mapmaking/destriper.py Type annotations, datetime.now with timezone
litebird_sim/mbs/mbs.py Fixed typo (allow_pickle) and variable reference bug
test/* Type annotation improvements in test files
notebooks/* Updated with getattr patterns and type annotations
Various other modules Consistent type annotation improvements

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

@ziotom78
Copy link
Member

Giacomo, thank you so much for having undertook this ordeal! I know this must have been tedious work, especially considering the broad extent of the changes. It’s impressive to see how many inconsistencies you’ve managed to fix.

I’m particularly pleased to see that ty caught auctual bugs, often in seldomly-run code paths. Seeing it propagate architectural changes like the new SphericalHarmonics class really reveals how useful it is for a large codebase like LBS. And it’s great that your modifications improve the documentation and adds robustness via several asserts. I believe this shows that ty effectively enforces code clarity.

Regarding the specific changes:

  • I noticed we used npt.ArrayLike far too often when npt.NDArray was more appropriate. (It seems I am one of the heaviest offenders here…) Thanks for fixing so many inconsistencies.

  • Correcting cases where types did not match (like list vs. non list) or parameters were missing Optional or None despite having a None default is important: even if these hints do not change execution, they improve our documentation tremendously. I als agree with your choice to ignore unresolved-import and the Numba issues: it’s better to be pragmatic here. Excluding docs/, benchmarks/, and the legacy mbs/ folder also makes perfect sense.

  • I was happily surprised that ty caught instances of overwriting constants (like lbs.PTEP_IMO_LOCATION). It’s also great to see it can handle Jupyter notebooks!

I have one technical curiosity: in several places, you switched to getattr instead of using the dot, e.g., getattr(mpi4py, "__version__") instead of mpi4py.__version__. Do you know why ty was complaining about the latter?

Looking ahead, this PR makes me think of two aspects of our workflow:

  • You anticipated my concern about CI efficiency! I agree that the pre-commit hook might be too strict if it blocks every single commit while someone is still in “creative mode”. Let’s try it for a while, but if becomes a bottleneck, we can consider moving the type check exclusively to the CI level.

  • BTW, Since it can be frustrating to run full type checks on every single commit pushed to a PR (sometimes you really want to commit stuff that is not 100% clean year), perhaps we should consider running these checks only when a PR is being merged. While the [skipci] trick in the commit message can be used here, not everyone is familiar with it. Moreover, it would save a huge number of CPU time, which is important because in our free plan the limit is not very high and was reached sometimes.

  • I see that Astral is releasing new versions of ty almost weekly. I would abstain from updating ty every time a new version is released; we could just bump the version if we stumble upon a bug, or if it is part of a larger update.

Excellent work. On my side, this should be merged ASAP, so that people can rebase their copy on this.

@paganol paganol merged commit 3c5a112 into master Jan 25, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants