Conversation
…inference methods for stride and downsampling
There was a problem hiding this comment.
I'm not quite sure what this is, but I suspect it shouldn't be included
| endif (OPENMP_FOUND AND NOT Katydid_SINGLETHREADED) | ||
|
|
||
| # ONNX Runtime | ||
| list (APPEND ONNX_LIBRARY_DIRS ${PROJECT_SOURCE_DIR}/External/onnxruntime/lib) |
There was a problem hiding this comment.
Since these aren't actually lists, you can just use set() to set a variable value.
However, it looks like these variables are only used in this section of the CMake structure, so you probably don't even need to worry about making variables.
| list (APPEND ONNX_LIBRARIES onnxruntime) | ||
| include_directories (${ONNX_INCLUDE_DIRS}) | ||
| link_directories (${ONNX_LIBRARY_DIRS}) | ||
| pbuilder_add_ext_libraries (${ONNX_LIBRARIES}) |
There was a problem hiding this comment.
I haven't looked into this package in detail yet, but it looks like you're including built libraries in the source tree. It'd be better to have just the source and build the libraries as part of the Katydid build. This will allow it to work on platforms for which the libraries that you pre-built aren't compatible.
|
|
||
| KTUnetSpectrumDiscriminator::KTUnetSpectrumDiscriminator(const std::string &name) : KTProcessor(name), | ||
| fModelFilePath("trained_unet.onnx"), | ||
| fInferenceOption("stride"), |
There was a problem hiding this comment.
Let's reduce the indentation here to 8 spaces or so. Makes it much more readable.
|
|
||
| namespace | ||
| { | ||
| const std::set<std::string> kInferenceOptions = {"stride", "downsampling"}; |
There was a problem hiding this comment.
I'm curious why this was defined this way. Another option would be to have it as a static member of the class.
| KTERROR(sdlog, "Null configuration node provided. Configuration failed."); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
For most of these parameters I'd suggest the following pattern:
fParameter = node->get_value("parameter", fParameter);
This should work well for you especially since you have some logic at the end of the function to validate the parameters. Give default values to these parameters that will fail those checks if the user doesn't set them if you want the user to have to set them.
In other cases, where there's a parameter that most of the time the user shouldn't have to worry about, the default value can be the value that works most of the time.
| MEMBERVARIABLE_NOSET(std::string, SamplingMethod); | ||
|
|
||
| private: | ||
| bool ReadModel(); |
There was a problem hiding this comment.
I recommend leaving these functions public. It makes them easier to write unit tests for. You can put them down lower in the header file if the user shouldn't need to use them.
| MEMBERVARIABLE_NOSET(unsigned, OutputWidth); | ||
| MEMBERVARIABLE_NOSET(unsigned, OutputHeight); | ||
| MEMBERVARIABLE_NOSET(std::string, SamplingMethod); | ||
|
|
There was a problem hiding this comment.
It'd be great to have some brief doxygen-compatible comments to explain what the different functions do.
| - "multi-ps": void (Nymph::KTDataPtr) -- accepts KTMultiPSData input and runs inference | ||
|
|
||
| Signals: | ||
| - "discrim-multi-ps": void (Nymph::KTDataPtr) -- emits the discriminated KTMultiPSData |
There was a problem hiding this comment.
We need to be outputting a distinct type from the input. The other classes called "[some sort of]SpectrumDiscriminator" output a collection of points that crossed the threshold. We should think about whether this should do something similar, or whether you do in fact want to output the equivalent of a KTMultiPSData.
2 participants
No description provided.