tickets/DM-41840: Implement improved detection algorithms #7
Conversation
fred3m
force-pushed
the
tickets/DM-41840
branch
6 times, most recently
from
474619d
to
2648153
Compare
fred3m
force-pushed
the
tickets/DM-41840
branch
from
2648153
to
33ee0b4
Compare
|
|
||
|
|
||
| class FreeFormComponent(FactorizedComponent): | ||
| class FactorizedFreeFormComponent(FactorizedComponent): |
There was a problem hiding this comment.
A complete rename of a class like this should at least be a deprecation with an alias until it's removed, if not an RFC.
There was a problem hiding this comment.
Normally I would agree but nothing under models has ever been used in production (and still isn't with this ticket). AFAIK no one other than me and a grad student that I work with have ever used this code, as it's all been experimental up until now. Deprecation also doesn't work, because I used the name for an actual FreeFormComponent (with no spectral constraints).
There was a problem hiding this comment.
As I wrote in #software-dev on LSSTC, you should be able to do this:
from deprecated.sphinx import deprecated
class FactorizedFreeFormComponent(FactorizedComponent):
...
@deprecated(reason="Renamed to FactorizedFreeFormComponent", version="v28.0", category=FutureWarning)
class FreeFormComponent(FactorizedFreeFormComponent):
pass
It's up to you but seeing as this class has been on main for over a year, I think it's best to deprecate the name properly.
There was a problem hiding this comment.
I understand your point and normally I would just deprecate it (since it isn't that hard). But in this case I actually reuse the name for a general, non-factorized FreeFormComponent (see https://github.com/lsst/scarlet_lite/blob/tickets/DM-41840/python/lsst/scarlet/lite/models/free_form.py#L143-L242). Had this been in broader use I probably would use a temporary name for the new class, with a deprecation warning on both, but I feel pretty strongly that in this instance no one even knows about these classes other than me and it's only a minor inconvenience to someone if I'm wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
- Coverage 94.85% 94.06% -0.79%
==========================================
Files 38 38
Lines 4740 4852 +112
==========================================
+ Hits 4496 4564 +68
- Misses 244 288 +44 ☔ View full report in Codecov by Sentry. |
No description provided.