JP-3749: Add mypy type checking #8852
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
assign_wcs
associations
extract_1d
stpipe
testing
installation
automation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8852 +/- ##
==========================================
- Coverage 61.79% 61.79% -0.01%
==========================================
Files 377 377
Lines 38832 38827 -5
==========================================
- Hits 23997 23992 -5
Misses 14835 14835 ☔ View full report in Codecov by Sentry. |
emolter
changed the title
|
Some failures in the first round of regtests related to over-zealous removal of things in extract1d, will look at this tomorrow or Monday and fix it |
|
new run of regression tests here after fixing the bug locally edit: all passing |
melanieclarke
approved these changes
Oct 10, 2024
jhunkeler
reviewed
Oct 10, 2024
jhunkeler
approved these changes
Oct 11, 2024
emolter
requested review from
tapastro
and removed request for
braingram,
zacharyburnett and
tapastro
hayescr
pushed a commit
to hayescr/jwst
that referenced
this pull request
Oct 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves JP-3749
Closes #8794
This PR adds type hint checking to the GitHub Actions workflow. The mypy configuration is nearly identical to the one in stcal, except that it disables checking imports globally instead of module-by-module. In order to bring the repository up to the standard required by these mypy checks, this PR also:
typing: ignoreflag in a few places, hopefully judiciously and with good reasonBefore all the mypy checks can pass, there are a few outstanding issues that need to be resolved:
__all__injwst.datamodelsgets lots of classes fromstdatamodels.jwst.datamodelsappended to it. I think the discussion in MyPy not acknowledging __all__ python/mypy#1606 may be relevant, but I can't figure out a good solution that doesn't involve manually ignoring all ~20 places in the repository where we import a specific model type fromdatamodels. Edit: I did decide to just manually ignore the 14 instances of this error. If someone has a better idea during PR review, I'm all ears.set_telescope_pointingwas type-hinted at some point, but the hints are either outdated or were never checked by mypy. These look like a fair amount of work to fix - perhaps removing them entirely is the best plan. Need to figure out if there would be objections to that. Edit: After discussing with @stscieisenhamer, I removed all type hints not directly related to the two dataclasses in there, and then found a sneaky way to make mypy ok with setting many of the attributes to None. There may still be a better solution but this one seems to work, at least.Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst