DM-32034: Create MatchProbabilisticTask #157
Conversation
| columns_target_select_false = pexConfig.ListField( | ||
| dtype=str, | ||
| default=('merge_peak_sky',), | ||
| doc='Target table columns to require to be True for selecting match candidates', |
| finite = np.isfinite(chi) | ||
| n_finite = np.sum(finite, axis=1) | ||
| chisq_good = n_finite >= config.match_n_finite_min | ||
| if np.any(chisq_good): |
There was a problem hiding this comment.
So does this mean you don't look at any object with NaN errors even if other columns are valid?
There was a problem hiding this comment.
Hmm, I suppose that's true. I guess I could add a min/max/default error or something, but then the user can also just modify the catalogs before passing them in.
There was a problem hiding this comment.
Would it be possible to only compare the non-NaN columns?
There was a problem hiding this comment.
It's already based on the non-NaN columns, and will succeed as long as there are more than n_finite finite chi values. It's just that it doesn't distinguish between measurements vs errors being NaN (but again, the user can modify the input catalogs if they care, and we really shouldn't be producing finite values with NaN errors in the first place).
| ): | ||
| self.config = config | ||
|
|
||
| def match( |
There was a problem hiding this comment.
Code could use some inline comments describing what is happening at a give step.
There was a problem hiding this comment.
Also, would it make sense to break some of this function up into sub sections? That would certainly make the testing easier.
There was a problem hiding this comment.
I don't think there would be much benefit in breaking it up, as any of the sub-functions I could think of defining would have large numbers of arguments/return values but still not really make sense to call in any other context.
| class MatchProbabilisticTask(pipeBase.Task): | ||
| """Run MatchProbabilistic on a reference and target catalog covering the same tract. | ||
| """ | ||
| ConfigClass = MatchProbabilisticConfig |
There was a problem hiding this comment.
Are there no configs specific to the task? Would it make more sense to have the prob matcher be a configurable subtask of the main task? It's fine if not.
There was a problem hiding this comment.
Just to clarify, I could have folded the Matcher into the Task instead of having a separate class, but this way the Matcher has no stack dependencies besides pex_config (which I believe can be installed as a standalone package itself), and in principle can be imported independently without building the package. Whether that ever ends up being useful to anyone remains to be seen.
There was a problem hiding this comment.
Sounds good. Like I said on the other repo, I think you already take care of this by having the configurable in the pipeline class.
taranu
force-pushed
the
tickets/DM-32034
branch
from
0cc92e2
to
2502ce6
Compare
| for column in config.columns_target_select_false: | ||
| select_target &= ~catalog_target[column].values | ||
|
|
||
| logger.info(f'Beginning MatcherProbabilistic.match with {np.sum(select_ref)}/{len(select_ref)}' |
There was a problem hiding this comment.
Please do not use f-strings with loggers. The logger must use % formatting and must be given as parameters at the end. The reason is to defer the stringification until we know we need it. There are four other places (at least) that need to be fixed.
taranu
force-pushed
the
tickets/DM-32034
branch
2 times, most recently
from
f7929f0
to
e72c3a3
Compare
taranu
force-pushed
the
tickets/DM-32034
branch
from
e72c3a3
to
a12e5d6
Compare
No description provided.