New inventory class #31
Conversation
| //! map of path to kernel pointers | ||
| std::vector<SharedKernel> loadedKernels; | ||
|
|
||
| //! json used to populate the loadedKernels | ||
| nlohmann::json kernels; | ||
| nlohmann::json m_kernels; |
There was a problem hiding this comment.
we need to be more consistent with these names 🙃
| @@ -928,14 +927,14 @@ namespace SpiceQL { | |||
| string currFile = fileType; | |||
|
|
|||
| //create a spice cell capable of containing all the objects in the kernel. | |||
| SPICEINT_CELL(currCell, 100); | |||
| SPICEINT_CELL(currCell, 200000); | |||
There was a problem hiding this comment.
changed to match ISIS, kept running into issues with large kernels e.g. smithed THEMIS kernels which have an insane amount of segments in them. Now that we are going to save the times off as binary, it almost doesn't matter if this is a waste of memory since it'll only run when creating the inventory.
| @@ -990,6 +989,104 @@ namespace SpiceQL { | |||
| } | |||
|
|
|||
|
|
|||
| pair<double, double> getKernelStartStopTimes(string kpath) { | |||
There was a problem hiding this comment.
Moved to only storing start/stop windows for the entire kernel, instead of saving every single segment like ISIS does. afaik we only care if a particular kernel has coverage.
| }; | ||
|
|
||
|
|
||
| class Database { |
There was a problem hiding this comment.
I should probably change this to InventoryImpl, or something more direct like that, as it might be mistaken for being related to the db files.
| class Database; | ||
|
|
||
| class Inventory { | ||
| Database *m_impl; |
There was a problem hiding this comment.
This is hiding the implementation, this way we don't have to expose types like BTreeMap to python or other languages.
| auto start_upper_bound = time_indices->start_times.upper_bound(stop_time); | ||
| for(auto it = time_indices->start_times.begin() ;it != start_upper_bound; it++) { | ||
| start_time_kernels.insert(it->second); | ||
| } | ||
|
|
||
| // Get everything stopping after the start_time; | ||
| auto stop_lower_bound = time_indices->stop_times.lower_bound(start_time); | ||
| for(auto &it = stop_lower_bound;it != time_indices->stop_times.end(); it++) { | ||
| // if it's also in the start_time set, add it to the list | ||
| iterations++; | ||
|
|
||
| if (start_time_kernels.contains(it->second)) { | ||
| final_time_kernels.push_back(time_indices->index.at(it->second)); | ||
| } | ||
| } |
There was a problem hiding this comment.
indices didn't help as much as I would have hoped because of the nature of these time windows. In practice, it cuts the amount of comparisons such that you're only checking half the times or less. Which is still better, I think most of the speed up is from using mmap binary files for reading and better truncation of kernels to search before doing any time searches.
| Config config; | ||
| json json_kernels = getLatestKernels(config.get()); |
There was a problem hiding this comment.
Ideally, this code would be integrated into Config, but didn't have time to do that level of a rewrite. Considering this is more in line with the API I envisioned when this first started, I wonder if we can make the Config classes private in the interface. That is, not expose it in the headers and by extension the Python API.
There was a problem hiding this comment.
Overall looks fine, from what I can tell the new Inventory and Database classes aren't used anywhere in the code yet. I could be missing something so please let me know otherwise.
I would also like to see tests for some of the functions added under utils, as well as more individual tests for the database class. I think the current Database class test depends on locally data and also doesn't make any test assertions
SpiceQL/tests/UtilTests.cpp
Outdated
| Database db(false); | ||
| db.write_database(); | ||
| nlohmann::json kernels = db.search_kernels("lroc", {Kernel::Type::FK, Kernel::Type::SCLK, Kernel::Type::SPK, Kernel::Type::CK, Kernel::Type::PCK}, Kernel::Quality::RECONSTRUCTED, 723470470.1831709, 723470590.1831709); | ||
| cout << kernels << endl; |
There was a problem hiding this comment.
Can this be expanded to check for some expected values?
…rch for lower qualities when requested isn't avaialable
Still needs tests and some clean up, up for initial review.
local tests shows massive speed improvements to kernel search, before using the old method was 20,000ms and using the new Inventory class was 4ms.