Update comments.

native/src/api.cpp

@@ -1243,8 +1243,10 @@ public:
 
 #if 0
 
-// TODO-Compression, since the factories have variable argument lists I might
+// TODO-Low: Refactor to a cleaner way of choosing a compressor.
+// Since the factories have variable argument lists I might
 // not be able to encapsulate as much as I do in the Python version.
+// Ummm... didn't I fix that by using a string list?
 
 //def ZgyCompressFactory(name, *args, **kwargs):
 //    return _internal.CompressFactoryImpl.factory(name, *args, **kwargs)

native/src/impl/bulk.cpp

@@ -236,7 +236,7 @@ struct WriteNowArgPack
        << ", size=" << rawdata.second;
     if (fileoffset)
       ss << ", fileoffset=" << std::hex << fileoffset << std::dec;
-    // TODO-Low: Symbolic names for enums
+    // Should use symbolic names for enums, but this is just for verbose logs.
     ss << ", brickstatus=" << (int)brickstatus;
     return ss.str();
   }
@@ -1014,8 +1014,8 @@ ZgyInternalBulk::_deliverOneBrick(
     break;
   }
   case BrickStatus::Normal: {
-    // TODO-Medium: byteswap, maybe clone first if not owndata.
-    // TODO-High: Casting away the const is ugly.
+    // TODO-Low: byteswap here. maybe clone first if not owndata.
+    // TODO-Low: Casting away the const is ugly. DataBuffer not const-correct.
     data = DataBuffer::makeDataBuffer3d(std::const_pointer_cast<void>(raw), rawsize, bricksize, dtype);
     if (_metadata->fh().version() == 1) {
       // Rare case. Does not need to be performant.
@@ -1027,7 +1027,7 @@ ZgyInternalBulk::_deliverOneBrick(
     break;
   }
   default:
-    // TODO-Medium: byteswap? Or did the constvalue decode do that?
+    // TODO-Low: byteswap? Or did the constvalue decode do that?
     data = DataBuffer::makeDataBuffer3d(std::const_pointer_cast<void>(raw), rawsize, bricksize, dtype);
   }
 
@@ -1219,12 +1219,6 @@ ZgyInternalBulk::_setPaddingSamples(
  * require changes at a lower level. Also, SegmentClosedExceptopn would
  * get really problematic to handle.
  *
- * TODO-Performance: Instead consider flushing multiple buffers in parallel,
- * or (more likely) use some parallel-upload features in GCS / Azure.
- * Or maybe have the lower level split each object apart from the one
- * with the headers into a fixed number of parts that can be uploaded
- * in parallel.
- *
  * For local writes, temporarily dropping a write lock while executing
  * the two xx_write() calls might help. But that requires a lot of testing.
  * Don't do it unless certain there will be a substantial benefit.
@@ -1328,8 +1322,8 @@ ZgyInternalBulk::_writeOneNormalBrick(const WriteBrickArgPack& args)
     }
   }
   if (brickstatus == BrickStatus::Compressed) {
-    // TODO-High, isn't it Normal data that needs byte swap?
-    // TODO-High, need to implement BYTE SWAPPING.
+    // TODO-Low, isn't it Normal data that needs byte swap?
+    // TODO-Low, need to implement BYTE SWAPPING.
     // TODO-High, in shortcut mode we might not own the buffer.
     // It might be safer to unconditionally copy the data.
     //data->byteswap();
@@ -1412,7 +1406,7 @@ ZgyInternalBulk::_mustLeakOldBrick(
     // compressed without changing the offset. There are
     // probably other problems as well.
     //
-    // TODO-High: Can I assume that the caller will veto the
+    // TODO-Worry: Can I assume that the caller will veto the
     // compression if an uncompressed brick already exists on the
     // file? If so then I don't need to complain about or leak the
     // "will become compressed" case because it won't.
@@ -1690,8 +1684,9 @@ ZgyInternalBulk::_writeAlignedRegion(
   // the "user" is really OpenZGY and not some client code. The only
   // acceptable error is ZgySegmentIsClosed, and that will be caught
   // and handled at lower levels.
-  // TODO-Low: Might implement retrying of writes at a lower level.
+  // Might implement retrying of writes at a lower level.
   // In that case we still shouldn't see those errors here.
+  // There doesn't seem to be anything to gain fron that change.
 
   // The _writeWithRetry() method is not threadsafe and *must* be
   // called sequentially. It is also highly recommended to write
@@ -1762,7 +1757,7 @@ ZgyInternalBulk::writeRegion(
                            _metadata->ih().datatype()))
     throw OpenZGY::Errors::ZgyUserError("Invalid data type in writeRegion");
 
-  // TODO-Performamce: Combining range() and _scaleDataToStorage()
+  // TODO-Performance: Combining range() and _scaleDataToStorage()
   // might save some time.
 
   if (!is_storage) {

native/src/impl/compress_zfp.cpp

@@ -149,7 +149,9 @@ private:
     const int precision = _zfp_precision_from_snr(idata, want_snr);
     const zfp_type type = zfp_type_float; // array scalar type
 
-    // TODO-High Am I expcted to transpose the data?
+    // TODO-Low Am I expcted to transpose the data?
+    // It might not make any practical difference as long as it is
+    // done consistently. More testing would be nice though.
     // Allocate meta data for the 3D uncompressed array a[nz][ny][nx]
     // Note that the FIRST size argument is the fastest varying one.
     // TODO-Low: Check at runtime that shape[n] fits in an integer.
@@ -216,7 +218,9 @@ private:
     rawdata_t result{nullptr,0};
     const zfp_type type = zfp_type_float; // array scalar type
     zfp_field* field = zfp_field_alloc();
-    // TODO-High Do I need to transpose or alternatively set strides?
+    // TODO-Low Do I need to transpose or alternatively set strides?
+    // It might not make any practical difference as long as it is
+    // done consistently. More testing would be nice though.
 
     // Allocate meta data for a compressed stream. Bitstream attached later.
     zfp_stream* zfp = zfp_stream_open(nullptr);

native/src/impl/enum.h

@@ -25,9 +25,6 @@
  * \details Some of the enums are used to describe parts of the file
  * format, giving a symbolic name to an integer stored in the file.
  * That kind of information should definitely be hidden from clients.
- * \internal TODO-Low: Move the typedefs to a separate file so that
- * code that only wants the enums won't need to pull in a lot of stl
- * stuff.
  */
 
 namespace InternalZGY {

native/src/impl/file.h

@@ -274,7 +274,8 @@ private:
  *
  * Thread safety: By design, all FileADT specializations are expected to
  * allow concurrent reads but no guaranteed about anything else.
- * TODO-High: Need to analyze individual methods for thread safety issues.
+ * TODO-Worry: Need to analyze individual methods for thread safety issues.
+ * This has been done. But maybe something slipped thru the cracks.
  */
 class FileCommon : public FileADT
 {

native/src/impl/file_consolidate.cpp

@@ -224,8 +224,6 @@ ConsolidateRequests::_split_requests(
       bool consolidate_overlaps,
       std::int64_t eof)
 {
-  // TODO-Low: In the Python code some parameters are inherited from
-  // calling method; this is confusing and wasn't actually intended.
   ReadDoubleList all_requests;
   std::int64_t prev_end = 0;
   ReadList sorted_requests = requests;

native/src/impl/file_sd.cpp

@@ -40,7 +40,9 @@
 #include <mutex>
 #include <omp.h>
 
-#ifndef _WIN32 // TODO-Low: SDAPI/ prefix also on Linux.
+// It would have been nice to have similar include paths in Linux and Windows
+// but that is a cosmetic issue only and it is only a problem in this file.
+#ifndef _WIN32
 #include <SDManager.h>
 #include <SDGenericDataset.h>
 #include <SDUtils.h>
@@ -205,7 +207,7 @@ private:
   };
   typedef std::vector<RawRequest> RawList;
   RawList _split_by_segment(const ReadList& requests);
-  void _cached_read(/*TODO-SeismicStore seg, offset, view*/);
+  void _cached_read(/*TODO-Low: seg, offset, view*/);
 private:
   OpenMode _mode;
   // TODO-Low: To improve isolation, the user visible context should
@@ -396,12 +398,15 @@ DatasetInformation::DatasetInformation(seismicdrive::SDGenericDataset* sdgd)
     }
 #if 0 // Chicken...
     else if (nblocks == 1) {
-      // WARNING: getSize is less reliable.
-      // I need to trust that each block is only written once.
-      // Or that if rewritten, caller passed check_and_overwrite=true.
-      // TODO-Low: SDAPI should really have been able to figure out by itself
-      // whether the check is needed.
-      // TODO-Worry: ensure that ZGY-Cloud doesn't need that flag.
+      // Not using getSize() because I do not trust it.
+      // SDAPI requires a hint check_and_overwrite=true when writing
+      // a block that might exist already. If the hint is missing
+      // then getSize() will return the wrong result. OpenZGY has
+      // no control over who wrote the file. Defensive programming
+      // says to just don't use the getSize() until the current
+      // behavior (which I consider a bug) is fixed. Suggestion:
+      // would it be possible to scan the file on exit and fix up
+      // any incorrect size?
       sizearray.push_back(nbytes);
     }
 #endif
@@ -493,7 +498,7 @@ DatasetInformation::allSizes(std::int64_t open_size) const
 
 /**
  * Do consistency checks before data is written.
- * TODO-Low: Parts might be redundant due to checks in caller.
+ * Some of these might be redundant due to checks in the caller.
  * E.g. caller raises SegmentIsClosed if attempting to write "backwards".
  * Throws exceptions on error.
  */
@@ -522,6 +527,7 @@ DatasetInformation::checkOnWrite(std::int64_t blocknum, std::int64_t blocksize)
   else if (blocknum + 1 == block_count_) {
     // Overwrite the last block, which is not block 0.
     // TODO-Low: Technically I might have allowed this.
+    // If update is to be supported then I probably need to.
     if (blocksize != last_block_size_)
       throw OpenZGY::Errors::ZgyInternalError("Cannot change the size when re-writing the last block");
   }
@@ -684,7 +690,7 @@ DatasetInformation::getLocalOffset(std::int64_t offset, std::int64_t size, std::
  *    seismicdrive::SDGenericDataset& dataset = *wrapper->dataset(); // NO!!!
  *    foo(dataset); // the returned smart pointer is already deleted.
  *
- * TODO-Yagni: _virgin is not used. It is related to the CTag mechanism.
+ * TODO-Low: Yagni: virgin_ is not used. It is related to the CTag mechanism.
  * It is still being discussed whether that needs to be ported from the
  * old accessor. It might not be needed if we go for immutable ZGY files.
  */
@@ -1172,29 +1178,20 @@ SeismicStoreFile::xx_readv(const ReadList& requests, bool parallel_ok, bool immu
   // are not supposed to know that SeismicStoreFileDelayedWrite makes
   // the file look bigger.
   //
-  // This implementation may in the future issue requests in multiple
+  // This implementation can issue requests in multiple
   // threads, wait for all threads to complete, and then deliver all
   // the results. For this reason it needs to allocate a buffer to
   // hold the entire data to be read.
   //
-  // TODO-Performance: multi-threading internally in xx_readv() for
-  // those cases where we end up requesting more than one chunk. This
-  // is already implemented in the Python version. This might not be
-  // useful if the application is multi threaded and is expected to
-  // issue ZGY read requests in parallel. (which is not supported in
-  // Python, so the feature makes more sense there). On the other
-  // hand, doing multi threading here instead means that the
-  // application doesn't need to guess which access pattern gives the
-  // largest chunks to be read.
-  //
-  // TODO-Performance: Allocate a buffer for each request in new_requests.
-  // Deliver results as soon as they become available. This facilitates
-  // reading and decompressing in parallel. Note that this change is
-  // probably not useful unless threading is implemented as well.
-  // CAVEAT: Requests that cross segment boundaries will then need some
-  // complicated "partial delivery" mechanism. Or the entire request may
-  // need to fall back to the original implementation if a boundary crossing
-  // (which is likely to be very rare) is detected.
+  // TODO-Performance: Allow read from cloud and copy-out/decompress
+  // in parallel inside a single request from the application. Probably
+  // not worth the (significant) trouble, and probably won't help
+  // multi-threaded applications anyway. Theoretically it might help
+  // lowres computation on write. The main caveat is that requests that
+  // cross segment boundaries will then need some complicated "partial
+  // delivery" mechanism. Or the entire request may need to fall back
+  // to the original implementation if a boundary crossing, which is
+  // likely to be very rare, is detected.
 
   std::int64_t current_eof = SeismicStoreFile::xx_eof(); // exclude open segment
   _validate_readv(requests, current_eof, this->_mode);
@@ -1215,7 +1212,6 @@ SeismicStoreFile::xx_readv(const ReadList& requests, bool parallel_ok, bool immu
   // Carefully get the required buffer size. Normally it would be
   // enough to just look at work.back() but there may be some odd
   // corner cases, and I just don't want to worry about those.
-  // TODO-Medium backport my paranoia to the Python version.
 
   const std::int64_t realsize =
     std::accumulate(work.begin(), work.end(), std::int64_t(0),
@@ -1228,11 +1224,8 @@ SeismicStoreFile::xx_readv(const ReadList& requests, bool parallel_ok, bool immu
   if (this->_config->_debug_trace)
     this->_config->_debug_trace("readv", /*need=*/asked, /*want=*/realsize,/*parts*/ work.size(), this->_dataset->info()->allSizes(-1));
 
-  // Do the actual reading, sequentially, one consolidated chunk at a time.
-  // TODO-Performance, can this easily be parallelized?
-  //
-  //  * I probably need a config option for max threads to avoid
-  //    overloading the data server.
+  // Do the actual reading of the consolidated chunks, possibly using
+  // multiple threads.
   //
   //  * Worry: Can there be multiple requests targeting the same area
   //    of the output buffer? Probably not although there can be multiple
@@ -1274,24 +1267,11 @@ SeismicStoreFile::xx_readv(const ReadList& requests, bool parallel_ok, bool immu
   guard.finished();
   //std::cerr << "$\n";
 
-  // TODO-Performance, if parallel_ok, can I parallelize only this
-  // loop if it gets too difficult to do it inside the above loop?
-  // This can help speed up ZgyInternalBulk::readToExistingBuffer().
-  //
-  //  * At this level. each request might be a large consolidated
-  //    read. This means that in addition to the code here, class
-  //    ConsolidateRequests::_consolidated_delivery might also need to
-  //    change. This is the functor responsible for breaking the jumbo
-  //    requests back into the original requests. It might also need
-  //    to do parallelizing. Which means issues with nested OpenMP
-  //    loops. One unpalatable alternative is to break encapsulation
-  //    and do some of _consolidated_delivery's processing here. Ouch.
-  //
-  //  * If I decide I won't even try to do the concurrent read and
-  //    process described earlier then there is a (I think) much
-  //    simpler alternative. Make a "parallelizer" adaptor that first
-  //    does all the reads and then returns them in parallel. I may
-  //    need to allocate yet another buffer for this, though.
+  // Do not try to multi-thread the following loop. Instead inject a
+  // FileParallelizer instance at a higher level. At this lowest level,
+  // each request might be a large consolidated read. Splitting and
+  // parallelizing the CPU bound tasks should be done at a finer one
+  // brick granularity and FileParallelizer is designed to do just that.
 
   std::int64_t pos = 0;
   for (const ReadRequest& rr : new_requests) {
@@ -1337,7 +1317,8 @@ SeismicStoreFile::xx_write(const void* data, std::int64_t offset, std::int64_t s
     this->_dataset->info()->getLocalOffset
       (offset, size, &blocknum, &local_offset, &local_size);
     // Normally we get here to overwrite blob 0, and that is ok.
-    // TODO-Low: This test will fail in the parallel upload case
+    // TODO-Low: This code needs more work if/when allowing update.
+    // This test will fail in the parallel upload case
     // because local_offset and local_size refers to SDAPI blocks and
     // not the larger segments that we are asked to write. local_size
     // will usually not be larger than one SDAPI block and will thus
@@ -1693,7 +1674,7 @@ SeismicStoreFile::_split_by_segment(const ReadList& requests)
 }
 
 void
-SeismicStoreFile::_cached_read(/*TODO-SeismicStore: seg, offset, view*/)
+SeismicStoreFile::_cached_read(/*TODO-Low: seg, offset, view*/)
 {
   throw std::runtime_error("SeismicStoreFile::_cached_read: not implemented yet");
 }
@@ -1719,7 +1700,8 @@ SeismicStoreFileDelayedWrite::SeismicStoreFileDelayedWrite(const std::string& fi
   this->_ctimer.reset(new SummaryPrintingTimerEx("Cloud.readcache"));
   this->_relay.reset(new SeismicStoreFile(filename, mode, iocontext));
 
-  // TODO-Low: The relayed file already did this.
+  // The relayed file already did this so we are making another copy.
+  // Not a big deal.
   auto context = dynamic_cast<const OpenZGY::SeismicStoreIOContext*>(iocontext);
   if (!context)
     throw OpenZGY::Errors::ZgyUserError("Opening a file from seismic store requires a SeismicStoreIOContext");
@@ -1847,6 +1829,7 @@ SeismicStoreFileDelayedWrite::xx_readv(const ReadList& requests, bool parallel_o
  * data cannot span the boundary between flushed and open segments,
  * and cannot cover both before and after EOF.
  * TODO-Low: Support can be added if it ever turns out to be useful.
+ * Possibly this may happen if/when support is added for update.
  * The only scenarios used today are overwrite entire segment 0
  * which will then always be closed. And append at EOF which will
  * obviously then not have date both before and after EOF and will
@@ -1857,8 +1840,8 @@ SeismicStoreFileDelayedWrite::xx_readv(const ReadList& requests, bool parallel_o
  *
  * Thread safety: NO. The method would need more information, such as
  * the size of all previous segments "in transit", in order to do this
- * correctly. On the other hand there is nothing preventing us to
- * split a segment in parts inside this method and write those parts
+ * correctly. On the other hand there is nothing preventing us from
+ * splitting a segment in parts inside this method and write those parts
  */
 void
 SeismicStoreFileDelayedWrite::xx_write(const void* data, std::int64_t offset, std::int64_t size, UsageHint usagehint)

native/src/impl/file_windows.cpp

@@ -14,7 +14,7 @@
 
 #ifdef _WIN32 // Entire file is windows only
 
-// TODO-Windows: Get rid of this temporary kludge.
+// TODO-Low: Get rid of this temporary kludge on Windows.
 #define _CRT_SECURE_NO_WARNINGS 1
 
 #include "file.h"

native/src/impl/genlod.cpp

@@ -654,11 +654,11 @@ GenLodImpl::_paste4(
  * the same algorithm. In the Python implementation this logic is in
  * HistogramData.suggestHistogramRange().
  *
- * TODO-Medium: Implement the zero-centric property for [2].
- *
  * TODO-Low: If data is being appended the code will still re-compute
- * the entire histogram. Include the current value range stored on
- * file in case the update only consists of smaller values. Problem
+ * the entire histogram. To do this, it uses the _written_sample_min/max
+ * kept track of while writing lod0 data. The range should be the union
+ * of the data range written previously, found in the old histogram, and
+ * the samples written this time around. Problem
  * is, the first write might not have bothered to finalize and thus
  * did not save this information. I probably need to "finalize" with
  * an empty histogram. Then include the existing histogram range in

native/src/impl/lodalgo.cpp

@@ -397,6 +397,10 @@ public:
 /**
  * \brief Weird algorithm, probably not useful.
  *
+ * TODO-Low: YAGNI: Too obscure to be useful.
+ * Removing this means the last three arguments to operator()
+ * can be removed.
+ *
  * Return either the minimum or the maximum value in a checkerboard pattern.
  *
  * Thread safety: Safe. Instance holds no data.
@@ -589,7 +593,7 @@ void createGenericLevelOfDetail(
 
   const bool needpad = core[0]<dsize[0] || core[1]<dsize[1] || core[2]<dsize[2];
 
-  // TODO-Low: this is temporary, see discussion later down.
+  // TODO-Low: Performance: This is temporary, see discussion later down.
   // If made permanent it should at least do a more efficient fill.
   if (needpad)
     for (std::int64_t ii = 0; ii < dsize[0]; ++ii)
@@ -632,14 +636,17 @@ void createGenericLevelOfDetail(
   if (needpad) {
     // Ambition levels to handle the last il/xl/sample if input size is odd:
     // 0 - Leave the samples unchanged. It is the callers responsibility to
-    //     padd input to even size or to fill the buffer with defaultvalue.
+    //     pad input to even size or to fill the buffer with defaultvalue.
     // 1 - Set the last sample to zero. Can be done here, but it is simpler
     //     albeit slightly more expensive to just zero the buffer first.
     // 2 - Set the last sample to simple decimation i.e. use just one input
     //     sample even when we have 2 or 4 available.
     // 3 - Use the available 1, 2, or 4 samples. Which is the "correct"
     //     solution but it is doubtful whether anybody will notice.
-    // TODO-Low choose what to do here.
+    //     *unless* we want to support 2d data by setting the brick size
+    //     of one of the dimensions to 1.
+    // TODO-Enhancement: Proper decimation at survey edge whan survey size
+    // is odd, and decimation when brick size in one of the dimensions is 1.
     if (core[0] < dsize[0])
       for (std::int64_t ii = core[0]; ii < dsize[0]; ++ii)
         for (std::int64_t jj = 0; jj < core[1]; ++jj)

native/src/impl/lookuptable.cpp

@@ -80,6 +80,11 @@ struct TmpLookupEntry
  * they are followed by some alpha tiles. This is harmless.
  * For aplha tiles the end offsets will be hopelessly wrong.
  * We will need to just assume 4 KB for those.
+ *
+ * TODO-Performance: The brick-end calculation can probably be skipped
+ * if the file has no compressed bricks. In that case the test for truncated
+ * file needs to be moved (fairly easy) and we would lose the test for
+ * overlapped bricks.
  */
 std::vector<std::uint64_t>
 LookupTable::calcLookupSize(

native/src/impl/meta.cpp

@@ -2085,7 +2085,7 @@ ZgyInternalMeta::flushMeta(const std::shared_ptr<FileADT>& file)
     // first few alpha tiles. We probably won't be writing those anyway.
     // TODO-Low: Should the padding be removed for the compressed case?
     // Not strictly needed there, but if writing directly to the cloud
-    // the padding ensures that all segments hace nice sizes.
+    // the padding ensures that all segments have nice sizes.
     // This can make life easier for a future OpenZGY with a cache module
     // when it wants to read the file that we are creating now.
     IHeaderAccess::podbytes_t allbytes;