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


+
+    // 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.

…to int overflow

Filename frequency fix

…files

…r consistency with the others

…e frequencies

… order

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.

…n beam subsets)

…_ptr is not guaranteed, tests now passing

Skycleaver tdb order fix

Prototype fix for threading of CB handler (WIP)

…into skycleaver

Split TDB output file into several files with fewer beams, version 2

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

coclar and others added 15 commits January 27, 2025 16:10

Add beam index to output dada headers

29021ad

Fixing incorrect indices when splitting tdb stream into multiple files

4937a65

Slightly clearer indexing when splitting tdb files by beam

6572ec1

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

Use std::copy when splitting output buffers by beam

cac3883

Fix bug with beam0_idx being uninitialised for IB

f2759e6

TDB output splitting now works if beams do not divide evenly between …

d57409c

…files

dashes instead of underscores in nbeams-per-file cmdline argument, fo…

61644a4

…r consistency with the others

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

Fix issue with SkyCleaver where tdb files were not being read in time…

3b6530b

… order

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

coclar and others added 14 commits March 5, 2025 10:12

Switch order of TDB output streams (loop first over coherent DMs, the…

77d5c6e

…n beam subsets)

Prototype fix for threading of CB handler (UNTESTED)

da9cea3

missed file in initial commit

20df23e

added missing capture on thread lambda

cce61f9

removed ThreadWrapper as order of construction destruction for unique…

e300fd3

…_ptr is not guaranteed, tests now passing

removed ThreadWrapper as order of construction destruction for unique…

c1bcd19

…_ptr is not guaranteed, tests now passing

Merge branch 'skycleaver' into tdb_output_splitting2

155abee

Fix missing comma in PipelineConfig.cpp

45b35de

Merge pull request #27 from hefehr/skycleaver_tdb_order_fix

60c441f

Skycleaver tdb order fix

Merge pull request #28 from erc-compact/threaded_CB_handler

3183229

Prototype fix for threading of CB handler (WIP)

Added filter feature to tiling generator

ba3c4cd

Merge branch 'skycleaver' of https://github.com/erc-compact/skyweaver …

ed85a1f

…into skycleaver

added code to split the delay models adn targets by beam set

7ec10a0

Merge pull request #23 from hefehr/tdb_output_splitting2

30734b6

Split TDB output file into several files with fewer beams, version 2



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

Incorporating skycleaver functionality to skyweaver #20

Are you sure you want to change the base?

Incorporating skycleaver functionality to skyweaver #20

Uh oh!

Conversation

vivekvenkris commented Sep 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewanbarr commented Mar 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants