Open
Conversation
robertu94
commented
Mar 6, 2026
Owner
Author
robertu94
left a comment
robertu94
left a comment
There was a problem hiding this comment.
@skyler-ruiter please take a look at this.
Comment on lines
+47
to
+56
| static std::vector<std::string> split_str(const std::string& s, char delim) { | ||
| std::vector<std::string> parts; | ||
| size_t start = 0, pos; | ||
| while((pos = s.find(delim, start)) != std::string::npos) { | ||
| parts.push_back(s.substr(start, pos - start)); | ||
| start = pos + 1; | ||
| } | ||
| parts.push_back(s.substr(start)); | ||
| return parts; | ||
| } |
Owner
Author
There was a problem hiding this comment.
You probably want std::ranges::views::split
Comment on lines
+70
to
+81
| static ParsedConnection parse_connection(const std::string& s) { | ||
| auto arrow = s.find(" <- "); | ||
| if(arrow == std::string::npos) | ||
| throw std::invalid_argument("Invalid connection (expected 'sN <- sM[:port]'): " + s); | ||
| ParsedConnection c; | ||
| c.to = trim_str(s.substr(0, arrow)); | ||
| std::string rhs = trim_str(s.substr(arrow + 4)); | ||
| auto colon = rhs.find(':'); | ||
| if(colon == std::string::npos) { c.from = rhs; } | ||
| else { c.from = rhs.substr(0, colon); c.port = rhs.substr(colon + 1); } | ||
| return c; | ||
| } |
Owner
Author
There was a problem hiding this comment.
I feel like there is a std::regex way to do this which is simpler
Comment on lines
+123
to
+144
| fzmodules_plugin& operator=(fzmodules_plugin const& rhs) { | ||
| if(this == &rhs) return *this; | ||
| libpressio_compressor_plugin::operator=(rhs); | ||
| pipeline_.reset(); | ||
| pipeline_dirty_ = true; | ||
| if(!stream_) cudaStreamCreate(&stream_); | ||
| copy_config_from(rhs); | ||
| return *this; | ||
| } | ||
|
|
||
| fzmodules_plugin& operator=(fzmodules_plugin&& rhs) noexcept { | ||
| if(this == &rhs) return *this; | ||
| libpressio_compressor_plugin::operator=(std::move(rhs)); | ||
| pipeline_ = std::move(rhs.pipeline_); | ||
| pipeline_dirty_ = rhs.pipeline_dirty_; | ||
| if(stream_) cudaStreamDestroy(stream_); | ||
| stream_ = rhs.stream_; | ||
| rhs.stream_ = nullptr; | ||
| move_config_from(rhs); | ||
| rhs.pipeline_dirty_ = true; | ||
| return *this; | ||
| } |
Owner
Author
There was a problem hiding this comment.
These shouldn't be needed. We should use a smart pointer to hold these various types that hides this detail from the plugin author. We can add these as utility classes.
| set(options, "pressio:abs", error_bound_); | ||
|
|
||
| // Global pipeline settings | ||
| set(options, "fzmodules:memory_strategy", memory_strategy_); |
Owner
Author
There was a problem hiding this comment.
What if the strategy is invalid?
Comment on lines
+167
to
+175
| for(size_t i = 0; i < stages_.size(); i++) { | ||
| auto parts = split_str(stages_[i], ':'); | ||
| if(parts[0] != "lorenzo") continue; | ||
| std::string sid = "s" + std::to_string(i); | ||
| auto it = lorenzo_params_.find(sid); | ||
| const LorenzoParams& p = (it != lorenzo_params_.end()) ? it->second : default_lorenzo_; | ||
| set(options, "fzmodules:" + sid + ":quant_radius", p.quant_radius); | ||
| set(options, "fzmodules:" + sid + ":outlier_capacity", p.outlier_capacity); | ||
| } |
Owner
Author
There was a problem hiding this comment.
Refactor this as a private member function
Comment on lines
+155
to
+160
| metrics.get("fzmodules:peak_memory", &peak_mem); | ||
| metrics.get("fzmodules:execution_time_us", &exec_us); | ||
| if(peak_mem) std::cout << " Peak GPU memory: " << (peak_mem/(1024.0*1024.0)) << " MB\n"; | ||
| if(exec_us) std::cout << " Time: " << exec_us << " μs (" | ||
| << (input.size_in_bytes()/(1024.0*1024.0)) / (exec_us/1e6) | ||
| << " MB/s)\n"; |
Owner
Author
There was a problem hiding this comment.
use the rusage module for this
Comment on lines
+177
to
+182
| std::cout << "Decompression quality:\n" | ||
| << " Max absolute error: " << stats.max_error << " (bound: " << eb << ")\n" | ||
| << " MSE: " << stats.mse << "\n" | ||
| << " PSNR: " << stats.psnr << " dB\n" | ||
| << " NRMSE: " << stats.nrmse << "\n\n"; | ||
| check(stats.max_error <= eb + 1e-5, "max error within bound"); |
Comment on lines
+3
to
+13
| // Tests a parallel-branch DAG pipeline: | ||
| // | ||
| // s0 : lorenzo:float:uint16 | ||
| // |-- codes --> s1 : diff:uint16 (difference coding on quant codes) | ||
| // |-- outlier_errors --> s2 : passthrough (identity path for outlier errors) | ||
| // | ||
| // Mirrors the C++ API usage: | ||
| // auto* diff = pipeline.addStage<DifferenceStage<uint16_t>>(); | ||
| // pipeline.connect(diff, lorenzo, "codes"); | ||
| // auto* pass = pipeline.addStage<PassThroughStage>(); | ||
| // pipeline.connect(pass, lorenzo, "outlier_errors"); |
Owner
Author
There was a problem hiding this comment.
There is a lot of duplication betweent he basic and dag versions. Could they be in the same file?
| #include "libpressio_ext/cpp/compressor.h" | ||
| #include "libpressio_ext/cpp/options.h" | ||
|
|
||
| static int g_failures = 0; |
Owner
Author
There was a problem hiding this comment.
Google Tests does a lot of this. You shouldn't need this.
| set(LIBPRESSIO_HAS_CUDA ON CACHE BOOL "Required by fzmodules" FORCE) | ||
| enable_language(CUDA) | ||
| find_package(CUDAToolkit REQUIRED) | ||
| find_package(FZModules REQUIRED) |
Owner
Author
There was a problem hiding this comment.
Question: do you have a spack module for this yet? We can add it to robertu94/spack_packages. Also we should consider a different name for this as this will be confusing for users.
- uses std::regex - made utility smart pointer classes to not need move/copy details - added mem strategy validation - moved libpressio details to private member functions - moved peak_mem and last_execution_time to get_metrics_impl - saw thread safety in in zfp mentioned so just put single since i wasnt thinking, changed it to serialized - got rid of stability option - updated documentation with suggestions - moved module details, added example, more overall / high-level - got rid of pressio:abs - added more docs about memory strategies - added stage examples for connections - changed pressio:abs to double - uses domain_manager() for memory management now - removes copy back to host - no need for unique_ptr for stages since they get managed in the pipeline with unique_ptrs - removes zero guards in get_metrics_results_impl - adds spaceship operator for lorenzoParams - and removes static - made cudart public - cmake was 3.0 maybe im working on an old version - updated test files - combined into one test file - used error_stat for needed stats - moved output behind verbose env variable - use printers.h functions - removes hardcoded path for runtime generated data - uses pressio_io_data_path_read for file io - and size module and time modules for those parts - and rusage - and removes unneeded test tracking
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.