Incorporating skycleaver functionality to skyweaver #20

vivekvenkris · 2024-09-25T10:22:49Z

Skycleaver functionality that takes f number of .tdb files and produces d*b number of filterbank files. Currently only works for total intensity.

…callbacks

…annel cfreq change

ewanbarr · 2024-09-26T09:32:01Z

cmake/compiler_settings.cmake

 # Set compiler flags
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC -fopenmp")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC -fopenmp -std=gnu++20")


C++ standard should be set in compiler_settings.cmake with

set (CMAKE_CXX_STANDARD 20)

removed this and added set (CMAKE_CXX_STANDARD 20)

ewanbarr · 2024-09-26T09:35:40Z

cpp/skyweaver/DescribedVector.hpp

@@ -213,6 +230,12 @@ struct DescribedVector {
     */
    auto const& operator[](std::size_t idx) const { return _vector[idx]; }

+
+    // also the at() method
+    auto& at(std::size_t idx) { return _vector[idx]; }


What purpose does this serve?

It is another function in std::vector that I tried to mimic here. Not required but good to have.

After discussion, we decided to keep this in.

ewanbarr · 2024-09-26T09:40:56Z

cpp/skyweaver/MultiFileWriter.cuh

+        : header_size(header_size), max_file_size(max_file_size),
+          stokes_mode(stokes_mode), output_dir(output_dir), prefix(prefix),
+          extension(extension), output_basename("") {};
+    MultiFileWriterConfig(MultiFileWriterConfig const& other)


Is there a reason for implementing these constructors? The class member variables are all either trivial types or types with RAII behaviour, so you shouldn't need to write custom copy constructors.

It was originally not RAII, but now they are, but the constructors were left over. I will delete them.

ewanbarr · 2024-09-26T09:41:04Z

cpp/skyweaver/MultiFileWriter.cuh

+          stokes_mode(other.stokes_mode), output_dir(other.output_dir),
+          base_output_dir(other.base_output_dir), prefix(other.prefix),
+          extension(other.extension), output_basename(other.output_basename) {};
+    MultiFileWriterConfig& operator=(MultiFileWriterConfig const& other)


see previous comment

Same as above.

ewanbarr · 2024-09-26T09:43:02Z

cpp/skyweaver/MultiFileWriter.cuh

+    MultiFileWriter(PipelineConfig const& config,
+                    std::string tag,
+                    CreateStreamCallBackType create_stream_callback);
+    MultiFileWriter(MultiFileWriterConfig config,


Why is this pass by value?

Should be const& or you will get a double copy

It can't be const as I will edit some values inside, but it can be pass by reference.

ewanbarr · 2024-09-26T09:46:48Z

cpp/skyweaver/ObservationHeader.hpp

+    double fch1 = 0.0;              // Centre frequency of the first channel
+    double foff = 0.0;              // Frequency offset between channels
+
+    bool sigproc_params = false;    // Whether to include sigproc parameters


Is this used for control somewhere? It seems to only be on the to_string call for the header.

ewanbarr · 2024-09-26T09:51:13Z

cpp/skyweaver/detail/MultiFileWriter.cu

+                                             CreateStreamCallBackType create_stream_callback)
+    : _tag(tag), _create_stream_callback(create_stream_callback)
+{
+    MultiFileWriterConfig writer_config;


Why do this rather than just writing the parameters on the _config? You already have a stack allocated instance.

Residual code from previous changes, will clean it up.

ewanbarr · 2024-09-26T10:00:04Z

cpp/skyweaver/src/ObservationHeader.cpp

+    std::size_t end = 0;
+    while(end != std::string::npos) {
+        end = str.find(',', start);
+        values.push_back(std::stof(str.substr(start, end - start)));


This could use some error handling, as the stof doesn't throw (see comments on hennings PR).

ewanbarr · 2024-09-26T10:01:49Z

cpp/skyweaver/src/SigprocHeader.cpp

+	std::vector<std::string> dec_s;      
+    boost::split(ra_s, ra_val, boost::is_any_of(":"));
+    boost::split(dec_s, dec_val, boost::is_any_of(":"));
+    ra = stod(boost::join(ra_s, ""));


Same comment on error handling here. as stod doesn't throw

Add .tmp suffix to output filenames, and remove it when they are closed.

…sf, neighbouring and overlapping beam generator

…e given by user

This reverts commit 820efb9.

This reverts commit cbaf03b.

…to int overflow

Filename frequency fix

…e frequencies

Optional output of stats and incoherent beam files

Fix integer overflow in SkyCleaver's expected bridge frequencies

ewanbarr · 2025-03-05T09:48:26Z

@vivekvenkris This request has gone on too long. At this point SkyCleaver is the defacto main branch.

vivekvenkris added 19 commits August 22, 2024 16:02

First draft of the skycleaver implementation

81bad82

Change source name and position from default values in header

efeffb7

seek to the correct timestamp

63c854b

use uint8 for filterbank output

0201f7e

Correct frequency tagging of tdb outputs

331c1cf

swap beam and dm loops, convert signed to unsigned by rescaling data

5dc0bf4

Merge commit '218f424562ce85a08af871551ef01183a00f91e0' into skycleaver

ccc6ebb

add has key, get_or_default methods to header

a823587

get-> get_or_default for few keys

e14fc64

add basefilename to multifilewriterconfig and change it as needed in …

4f72d08

…callbacks

reuse bytes_seeking

36b1271

reflect file writer callback changes in tester

b36cb9f

fix paths and filenames

4826368

check for both OBS_FREQ & OBS_FREQUENCY

4aec59b

proper naming, more header parameters to read from dada files

2462dad

get mjd_start from utc_offset

c55bf3c

fix snake naming bug

819ee96

actually fix snake naming bug

63b85f8

use long double for times, incorporate downstream changes for half ch…

510fc01

…annel cfreq change

vivekvenkris assigned ewanbarr Sep 25, 2024

Ewan Barr added 2 commits September 26, 2024 11:37

fixed formatting on MultiFileWriter.cuh

4ae82ab

formatting

2787d00

ewanbarr reviewed Sep 26, 2024

View reviewed changes

coclar and others added 7 commits September 26, 2024 16:03

Add .tmp suffix to output filenames, and remove it when they are closed.

6a33108

Merge pull request #21 from hefehr/temporary_filenames

bbd9fd1

Add .tmp suffix to output filenames, and remove it when they are closed.

cleave only required samples

456719a

cleave only required dms and beams, code review changes

48adc14

seek to start sample after dedisp delay correction

0cfe1f9

CDD all data including overlap

ee35ae2

fix FFT shift, change left over gulps -> fft_size where appropriate

52b1ab3

vivekvenkris and others added 24 commits October 11, 2024 17:40

Make skycleaver a CPP application

4bf4df1

correctly set frequencies for described vectors, CDD, IDD

70647a9

Added multiple tiling plot code, targets file, mosaic command file, p…

0dde728

…sf, neighbouring and overlapping beam generator

removed unused class

5ca8add

changed python delay creation code to plot, even if no known beams ar…

8fbb204

…e given by user

Remove .bdm.db files and update .gitignore

0595fa9

typo fixes in include flags, migration from cuh to hpp

00fa137

minor merge changes

e697b31

get header values from header instead of user

a1a2c94

dirty pass on scaling change

cbaf03b

compiling now and beamformer tests running

820efb9

linear pol output and actual beam positions in skycleaver

49c966a

fix filename convention

ff71497

draft documentation

45964ca

Revert "compiling now and beamformer tests running"

8610cab

This reverts commit 820efb9.

Revert "dirty pass on scaling change"

112a857

This reverts commit cbaf03b.

segregate filterbanks in beam directories

13047bb

Fix for incorrect S-band frequencies being reported in filenames due …

4f15b64

…to int overflow

Merge pull request #24 from hefehr/filename_freq_fix

fbb980b

Filename frequency fix

Fix for integer overflow in SkyCleaver while computing expected bridg…

03b92c5

…e frequencies

Added command-line option for computing/outputing voltage statistics

4242cf9

Added command-line option to disable writing the incoherent beam

6675ae6

Merge pull request #26 from hefehr/optional_outputs

ab6b13e

Optional output of stats and incoherent beam files

Merge pull request #25 from hefehr/skycleaver_freq_fix

0d8e6aa

Fix integer overflow in SkyCleaver's expected bridge frequencies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporating skycleaver functionality to skyweaver #20

Incorporating skycleaver functionality to skyweaver #20

vivekvenkris commented Sep 25, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 30, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 26, 2024

vivekvenkris Sep 30, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 30, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 30, 2024

ewanbarr Sep 26, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 30, 2024

ewanbarr Sep 26, 2024

ewanbarr Sep 26, 2024

vivekvenkris Sep 26, 2024

ewanbarr Sep 26, 2024

ewanbarr Sep 26, 2024

ewanbarr commented Mar 5, 2025

Incorporating skycleaver functionality to skyweaver #20

Are you sure you want to change the base?

Incorporating skycleaver functionality to skyweaver #20

Conversation

vivekvenkris commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanbarr commented Mar 5, 2025