[vsipl++] [patch] Scalable SAR benchmark
Stefan Seefeld
stefan at codesourcery.com
Mon Oct 30 15:01:49 UTC 2006
Don,
This looks good. I like the heavily commented / documented code.
That helps a lot in understanding what the code is doing !
I have some high-level / stylistic comments:
Don McCoy wrote:
> Index: apps/ssar/load_save.hpp
> ===================================================================
> --- apps/ssar/load_save.hpp (revision 0)
> +++ apps/ssar/load_save.hpp (revision 0)
> @@ -0,0 +1,114 @@
> +/* Copyright (c) 2006 by CodeSourcery. All rights reserved. */
> +
> +/** @file load_save.hpp
> + @author Don McCoy
> + @date 2006-10-26
> + @brief Extensions to allow type double to be used as the view
> + data type while using float as the storage type on disk.
I think it would be best to follow the same idiom we agreed on for view I/O
(and which we now use for our matlab reader / writer), e.g.
input_stream >> Decoder<Vector<double>, float>(view);
This would help us promote this idiom, and make documentation easier.
> +*/
> +
> +#ifndef LOAD_SAVE_HPP
> +#define LOAD_SAVE_HPP
> +
> +#include <vsip_csl/load_view.hpp>
> +#include <vsip_csl/save_view.hpp>
> +
> +using namespace vsip_csl;
> +
> +template <typename Block>
> +void
> +save_view(
> + char* filename,
This should be 'char const *'.
> + vsip::const_Matrix<complex<double>, Block> view)
> +{
> + vsip::Matrix<complex<float> > sp_view(view.size(0), view.size(1));
> +
> + for (index_type i = 0; i < view.size(0); ++i)
> + for (index_type j = 0; j < view.size(1); ++j)
> + sp_view.put(i, j, static_cast<complex<float> >(view.get(i, j)));
> +
> + Save_view<2, complex<float> >::save(filename, sp_view);
Where is the Save_view template defined ? I couldn't find it anywhere.
(I'm wondering whether this could be generalized to do the type cast
during the streaming, to avoid the above extra copy.)
> +vsip::Matrix<complex<double> >
> +load_view(
> + char* filename,
> + vsip::Domain<2> const& dom)
> +{
> + vsip::Matrix<complex<float> > sp_view(dom[0].size(), dom[1].size());
> + sp_view = Load_view<2, complex<float> >(filename, dom).view();
> +
> + vsip::Matrix<complex<double> > view(dom[0].size(), dom[1].size());
> +
> + for (index_type i = 0; i < dom[0].size(); ++i)
> + for (index_type j = 0; j < dom[1].size(); ++j)
> + view.put(i, j, static_cast<complex<double> >(sp_view.get(i, j)));
Same comment here. There must be a way to load the view without this extra
copy. I believe the matlab formatter allows that, too, IIRC.
> Index: apps/ssar/diffview.cpp
> ===================================================================
> --- apps/ssar/diffview.cpp (revision 0)
> +++ apps/ssar/diffview.cpp (revision 0)
> @@ -0,0 +1,110 @@
> +/* Copyright (c) 2006 by CodeSourcery. All rights reserved. */
> +
> +/** @file diffview.cpp
> + @author Don McCoy
> + @date 2006-10-29
> + @brief Utility to compare VSIPL++ views to determine equality
> +*/
> +
> +#include <iostream>
> +#include <stdlib.h>
> +
> +#include <vsip/initfin.hpp>
> +#include <vsip/math.hpp>
> +
> +#include <vsip_csl/load_view.hpp>
> +#include <vsip_csl/save_view.hpp>
> +#include <vsip_csl/error_db.hpp>
> +
> +
> +using namespace vsip;
> +using namespace vsip_csl;
> +using namespace std;
> +
> +
> +typedef enum
> +{
> + COMPLEX_VIEW = 0,
> + REAL_VIEW,
> + INTEGER_VIEW
> +} data_format_type;
What's the reason this is a typedef, as opposed to
enum data_format_type {...};
? (This looks like C-style programming :-) )
> +
> +static void compare(data_format_type format,
> + char* infile, char* ref, length_type rows, length_type cols);
Shouldn't these be 'char const *' (infile, ref) ?
Replace this use of 'static' with an unnamed namespace to get the
same effect. Though I'm not sure what the desired effect is, since
this is the main source file anyway...
> Index: apps/ssar/kernel1.hpp
> ===================================================================
> --- apps/ssar/kernel1.hpp (revision 0)
> +++ apps/ssar/kernel1.hpp (revision 0)
> @@ -0,0 +1,537 @@
> +/* Copyright (c) 2006 by CodeSourcery. All rights reserved. */
> +
> +/** @file kernel.hpp
> + @author Don McCoy
> + @date 2006-10-26
> + @brief VSIPL++ implementation of SSCA #3: Kernel 1, Image Formation
> +*/
> +
> +#include <vsip/impl/profile.hpp>
> +
> +#include "load_save.hpp"
> +
> +#if 0
> +#define VERBOSE
> +#define SAVE_VIEW(a, b) save_view(a, b)
> +#else
> +#define SAVE_VIEW(a, b)
> +#endif
> +
> +// Files required to be in the data directory:
> +#define SAR_DIMENSIONS "dims.txt"
> +#define RAW_SAR_DATA "sar.view"
> +#define FAST_TIME_FILTER "ftfilt.view"
> +#define SLOW_TIME_WAVENUMBER "k.view"
> +#define SLOW_TIME_COMPRESSED_APERTURE_POSITION "uc.view"
> +#define SLOW_TIME_APERTURE_POSITION "u.view"
> +#define SLOW_TIME_SPATIAL_FREQUENCY "ku.view"
Can these become
char const *SAR_DIMENSIONS = "dims.txt";
etc., instead ? (Let's not use macros more than necessary !)
> Index: apps/ssar/viewtoraw.cpp
> ===================================================================
> --- apps/ssar/viewtoraw.cpp (revision 0)
> +++ apps/ssar/viewtoraw.cpp (revision 0)
> @@ -0,0 +1,121 @@
> +/* Copyright (c) 2006 by CodeSourcery. All rights reserved. */
> +
> +/** @file viewtoraw.cpp
> + @author Don McCoy
> + @date 2006-10-28
> + @brief Utility to convert VSIPL++ views to raw greyscale
> +*/
> +
> +#include <iostream>
> +#include <stdlib.h>
> +
> +#include <vsip/initfin.hpp>
> +#include <vsip/math.hpp>
> +
> +#include <vsip_csl/load_view.hpp>
> +#include <vsip_csl/save_view.hpp>
> +
> +
> +using namespace vsip;
> +using namespace vsip_csl;
> +using namespace std;
> +
> +
> +typedef enum
> +{
> + COMPLEX_MAG = 0,
> + COMPLEX_REAL,
> + COMPLEX_IMAG,
> + SCALAR_FLOAT,
> + SCALAR_INTEGER
> +} data_format_type;
Same comment as above.
> +
> +static void convert_to_greyscale(data_format_type format,
> + char* infile, char* outfile, length_type rows, length_type cols);
Same comment(s) as above.
> Index: apps/ssar/ssar.cpp
> ===================================================================
> --- apps/ssar/ssar.cpp (revision 0)
> +++ apps/ssar/ssar.cpp (revision 0)
> @@ -0,0 +1,93 @@
> +/* Copyright (c) 2006 by CodeSourcery. All rights reserved. */
> +
> +/** @file ssar.cpp
> + @author Don McCoy
> + @date 2006-10-26
> + @brief VSIPL++ implementation of HPCS Challenge Benchmarks
> + Scalable Synthetic Compact Applications -
> + SSCA #3: Sensor Processing and Knowledge Formation
> +*/
> +
> +#include <iostream>
> +#include <fstream>
> +#include <errno.h>
This should be <cerrno>.
Thanks,
Stefan
--
Stefan Seefeld
CodeSourcery
stefan at codesourcery.com
(650) 331-3385 x718
More information about the vsipl++
mailing list