U/echarles/all the estimators #2
Conversation
| @@ -107,8 +101,15 @@ class EstimatePZAlgoConfigBase( | |||
| def estimator_class(cls) -> type[CatEstimator]: | |||
| raise NotImplementedError() | |||
|
|
|||
| _bands = pexConfig.ListField[str]( | |||
There was a problem hiding this comment.
Why underscore this? I don't think the average user will attempt to change it without any reason.
There was a problem hiding this comment.
Because 'bands' is already in use in some of the algorithms.
There was a problem hiding this comment.
And this is not actually used by the code, it is just a way to set the band names more easily.
There was a problem hiding this comment.
The bands field used elsewhere seems to be a list of magnitude column names like mag_r_lsst. Is RAIL reading this as an attribute or something?
There was a problem hiding this comment.
The various estimators have a variety of patterns as to how they get the list of colums to use. Some (but not all) of them have a parameter called bands to do this.
There was a problem hiding this comment.
That's what I mean, it's confusing to have it be called bands. It should be something like fluxes or flux_columns. If it has to be called bands because RAIL expects it to be called bands, then it would be best to explain this in the doc for the field.
Or put another way, the docs for both _bands and bands should explain to a user initializing one of the config classes that contain both what the difference between the two is, and what overriding them would do.
| "mag_z_lsst", | ||
| "mag_y_lsst", | ||
| ] | ||
| self.pz_algo.mag_limits = dict( |
There was a problem hiding this comment.
I suggest putting this in some common definitions file if it's meant to be shared between algorithms. If you foresee the values diverging then never mind.
There was a problem hiding this comment.
What do you have in mind here? Where would that file live, what would it contain, etc..? Do you have a example.
There was a problem hiding this comment.
On second thought, a better option is probably defining a class variable like mag_limits_default = dict(...) in EstimatePZTaskConfig, then subclasses can put self.pz_algo.mag_limits = self.mag_limits_default.
|
It is a config parameter of some of the algorithms. There are a few different patterns for how they get configured On Apr 3, 2025, at 6:21 PM, Dan Taranu ***@***.***> wrote:
@taranu commented on this pull request.
In python/lsst/meas/pz/estimate_pz_task.py:
@@ -107,8 +101,15 @@ class EstimatePZAlgoConfigBase(
def estimator_class(cls) -> type[CatEstimator]:
raise NotImplementedError()
+ _bands = pexConfig.ListField[str](
The bands field used elsewhere seems to be a list of magnitude column names like mag_r_lsst. Is RAIL reading this as an attribute or something?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: ***@***.***>
|
|
This isn’t really part of RAIL, this is part of the wrapper that prepares things for rail by converting fluxes to mags and applying de-reddening.
This variable can be called anything that makes sense that isn’t really in use by RAIL. `_bands` seemed reasonable to me, but `bands_to_convert` would also be fine.
… On Apr 4, 2025, at 12:33 PM, Dan Taranu ***@***.***> wrote:
@taranu commented on this pull request.
In python/lsst/meas/pz/estimate_pz_task.py <#2 (comment)>:
> @@ -107,8 +101,15 @@ class EstimatePZAlgoConfigBase(
def estimator_class(cls) -> type[CatEstimator]:
raise NotImplementedError()
+ _bands = pexConfig.ListField[str](
That's what I mean, it's confusing to have it be called bands. It should be something like fluxes or flux_columns. If it has to be called bands because RAIL expects it to be called bands, then it would be best to explain this in the doc for the field.
Or put another way, the docs for both _bands and bands should explain to a user initializing one of the config classes that contain both what the difference between the two is, and what overriding them would do.
—
Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRIGIUQS5HV4JY3MPF6SBL2X3NCPAVCNFSM6AAAAABZ6DLC7GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONBTHEZTGNBSGI>.
You are receiving this because you were assigned.
|
Added support for several other estimators, bpz, cmnn, dnf, fzboost, gpz, lephare, tpz.
Added tests for each running against DC2, HSC and ComCam data.