Refactor VaryingCelestialTransform models #344
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
+ Coverage 96.08% 97.50% +1.42%
==========================================
Files 51 34 -17
Lines 2832 1887 -945
==========================================
- Hits 2721 1840 -881
+ Misses 111 47 -64 ☔ View full report in Codecov by Sentry. |
Makes some things easier with the astropy modelling internals, which require parameters to be defined on the class, not the instance.
dkist/wcs/models.py
Outdated
|
|
||
| if not isinstance(projection, m.Pix2SkyProjection): | ||
| raise TypeError("The projection keyword should be a Pix2SkyProjection model class.") | ||
| self.projection = projection | ||
|
|
||
| if self.is_inverse: |
There was a problem hiding this comment.
I am conflicted about this. I like the reduction in classes, but I think that ultimately this might be more confusing when introspecting the models etc. The most common pattern with astropy models is that you have a different class for the inverse.
What would the fall out be from putting the inverse classes back as well?
There was a problem hiding this comment.
Minimal I should think. The way it was before there was some calulation happening in the inverse classes but now that's been brought in to the base class it would be pretty straightforward to just have an inverse class for each dimension that sets self.is_inverse = True and does nothing else. Some of the corresponding infrastructure would have to go back in but that's fine.
Happy to revert this later, just want to make sure everything else is working
This reverts commit 31cf57f.
dkist/wcs/models.py
Outdated
| @@ -1,5 +1,7 @@ | |||
| from abc import ABC | |||
| from typing import Union, Literal, Iterable | |||
| # from functools import cache | |||
dkist/wcs/models.py
Outdated
| "InverseVaryingCelestialTransform", | ||
| "InverseVaryingCelestialTransform2D", | ||
| "InverseVaryingCelestialTransform3D", |
Starts compressing WCS models into a single model that can handle multiple dimensions of input.