DM-29775: Fix style issues introduced in DM-29737 and log output for unsorted catalogs #582
Conversation
Unlike std::lower_bound, std::find guarantees that *i = value if the search is successful. Thus, the second condition on checking if *i != value is redundant.
| @@ -15,6 +15,7 @@ | |||
| #include "lsst/afw/table/io/FitsWriter.h" | |||
| #include "lsst/afw/table/io/FitsReader.h" | |||
| #include "lsst/afw/table/SchemaMapper.h" | |||
| #include "lsst/log/Log.h" | |||
There was a problem hiding this comment.
I couldn't find an example where logging is done from another header file. I guess it makes sense in general, but since the implementation of the find method is in this header file, and the info about whether or not the catalog was sorted is not propagated up, the logging has to be done from here (if it's done).
| @@ -95,7 +95,7 @@ void toNanojanskyVariance(ndarray::Array<float const, 2, 1> const &instFlux, | |||
| } | |||
|
|
|||
| double toMagnitudeErr(double instFlux, double instFluxErr, double scale, double scaleErr) { | |||
| return 2.5 / log(10.0) * hypot(instFluxErr / instFlux, scaleErr / scale); | |||
| return 2.5 / std::log(10.0) * hypot(instFluxErr / instFlux, scaleErr / scale); | |||
There was a problem hiding this comment.
This was necessary to use the natural logarithm and not a logging function, and whether or not we decide to log, we should perhaps keep this change.
include/lsst/afw/table/Catalog.h
Outdated
| return end(); | ||
| } | ||
| LOGL_INFO("afw.table.Catalog", "Catalog is not sorted by the key. Finding a record may be slow."); |
There was a problem hiding this comment.
One of the motivations for logging this at some low level is that this is (probably?) the only direct indication we have to let us know that the catalog is unsorted and I think it'd be useful to keep that at TRACE level at least.
There was a problem hiding this comment.
I'd probably recommend DEBUG for this.
| return end(); | ||
| } |
There was a problem hiding this comment.
Sorry I didn't catch these in my initial review; that's definitely on me (I think it was just a sufficiently busy day that I only looked at correctness and forgot to look at style the first time around).
There was a problem hiding this comment.
Totally understandable. I wasn't familiar with C++ style guidelines, so I made it a point to go back and check after merging, just in case.
arunkannawadi
force-pushed
the
tickets/DM-29775
branch
from
f813e5d
to
20ecdc1
Compare
This PR a) fixes a few style errors introduced in a recent merge DM-29737 and b) create a log message if the catalog is found to be unsorted. The log message is to hint the user of plausible inefficiencies in finding a desired record as a result of having catalog unsorted by the Id.