man: Improve segment library documentation, provide examples by wenzeslaus · Pull Request #20 · OSGeo/grass

wenzeslaus · 2019-05-24T03:22:55Z

This PR adds two examples into doc/examples which can be compiled as GRASS modules and executed. It adds 3 new sections to the library dox file: about general use with raster, about use with list of rasters (includes portions of one of the examples, so the code is only at one place), and a complete example (here including whole files). Finally, it modifies documentation for two functions.

Besides asking for general review of best practices in using this library, I'm asking these two questions:

Is the remark about value initialization correct? (I don't really see a point in using calloc instead of malloc there considering that the non-memory case does initialize with zeros.)
What about the precondition for Segment_get_row()? Does Segment_get() need the same one? Segment_flush() description suggests that only writing to disk and Segment_get_row() need that and Segment_get_row() does not call any page function if that has any influence.

metzm · 2019-05-24T06:31:50Z

+
+    /* determine the input map type (CELL/FCELL/DCELL) */
+    map_type = Rast_map_type(input_name, "");
+    size_t cell_size = Rast_cell_size(map_type);


move "size_t cell_size;" up, just after "RASTER_MAP_TYPE map_type;"

metzm · 2019-05-24T06:32:30Z

+    ncols = Rast_window_cols();
+
+    /* size of a segment */
+    int srows = 64;


Move "int srows, scols, num_seg;" up

metzm · 2019-05-24T06:33:16Z

+    int num_seg = 4;
+
+    /* segment structure */
+    SEGMENT raster_seg;


move "SEGMENT raster_seg;" up

metzm · 2019-05-24T06:35:48Z

+
+    /* pass the pointer, set the value */
+    Segment_put(raster_seg, values, row, col);
+}


G_free(values):

metzm · 2019-05-24T06:36:45Z

+    ncols = Rast_window_cols();
+
+    /* size of a segment */
+    int srows = 64;


as for r.example.segment, move int srows, sclos, nsegs; up

metzm · 2019-05-24T06:37:17Z

+    int nsegs = 4;
+
+    map_type = DCELL_TYPE;
+    size_t cell_size = Rast_cell_size(map_type);


Move "size_t cell_size, segment_cell_size;" up

metzm · 2019-05-24T06:37:28Z

+    output_fd = Rast_open_new(output_name, map_type);
+
+    /* segment structure */
+    SEGMENT raster_seg;


metzm · 2019-05-24T06:37:50Z

+        G_fatal_error(_("Unable to create temporary segment file"));
+
+    /* array to store file descriptors */
+    int *input_fds = G_malloc(ninputs * sizeof(int));


move "int *input_fds;" up

metzm · 2019-05-24T06:38:14Z

+    }
+
+    /* allocate input buffer */
+    DCELL *row_buffer = Rast_allocate_d_buf();


move "DCELL *row_buffer, *seg_buffer;" up

metzm · 2019-05-24T06:38:48Z

+
+    /* pass the pointer, set the value */
+    Segment_put(raster_seg, values, row, col);
+}


G_free(values);

metzm · 2019-05-24T06:39:19Z

+
+<em>
+<a href="r.example.html">r.example</a>
+<a href="r.example.html">r.example.segment</a>


r.example.segment.html

metzm · 2019-05-24T06:41:12Z

- * <b>Buf</b> will be filled with <em>ncols*len</em> bytes of data
- * corresponding to the <b>row</b> in the data matrix.
+ * \pre *buf* points to an allocated array of length <em>ncols*len</em>
+ * where *len* is size of one stored value (cell).


e.g. cell or a custom data structure

metzm · 2019-05-24T06:42:54Z

+ * Segment_init(), all-in-memory mode initialization, and temporary
+ * file handling.
+ *
+ * The values are not guaranteed to be initialized to zero or NULL.


not values, but bytes, they are indeed initialized to zero because of lseek and calloc

metzm · 2019-05-24T06:44:30Z

+the raster you need to access:
+
+\code
+for (int row = 0; row < Rast_window_rows(); row++) {


for (int row = 0; ...) is C++ style

metzm · 2019-05-24T06:47:47Z

+
+\skip G_free
+\until Rast_close(output_fd);
+


Segment_close() ?

metzm · 2019-05-24T06:54:28Z

This PR adds two examples into doc/examples which can be compiled as GRASS modules and executed. It adds 3 new sections to the library dox file: about general use with raster, about use with list of rasters (includes portions of one of the examples, so the code is only at one place), and a complete example (here including whole files). Finally, it modifies documentation for two functions.

Please use standard C style in sample code.
Maybe documentation does not need to be that long? avoid tl;dr

Besides asking for general review of best practices in using this library, I'm asking these two questions:
* Is the remark about value initialization correct? (I don't really see a point in using calloc instead of malloc there considering that the non-memory case does initialize with zeros.)

See "man calloc". malloc does not initialize the memory while calloc does.

* What about the precondition for `Segment_get_row()`? Does `Segment_get()` need the same one?

No.

Segment_flush() description suggests that only writing to disk and Segment_get_row() need that and Segment_get_row() does not call any page function if that has any influence.

Yes, that's the reason: Segment_get_row() bypasses the internal cache and reads directly from the temporary file.

metzm · 2019-05-24T20:23:51Z

Considering examples on how to use the segment library, it is IMHO sufficient to mention current working modules like r.cost, r.walk, r.watershed, r.stream.extract.

mlennert · 2019-05-29T12:34:42Z

Jumping into this discussion a bit late, so just two short comments:

I think we definitely need enhanced and detailed documentation of the C-code. @metzm, today you are probably the only one to really know the nitty-gritty stuff in this library, so for future sustainability (and possibly attracting the rare C-developer out there), good doc seems a must to me.
At this stage, I haven't had the chance to delve into the doc enough to understand whether new (simple) examples are needed. They have the advantage of focusing on the features documented, thus avoiding distraction through other unrelated parts of code. They have the disadvantage of having to be updated when the library changes. Actual production modules obviously provide more motivation to keep them up to date.

I don't think this has been done as such in the code doc, yet, but maybe we could just add specific references to the existing production modules directly into the doc ?

wenzeslaus · 2020-11-10T18:01:55Z

Note to myself: It seems that it worth explicitly noting that functions such as Segment_get() don't do bounds check for row and col (and thus crash will occur rather than an error message).

neteler · 2023-11-07T12:46:19Z

@wenzeslaus would you mind to rebase this PR?

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

echoix

@wenzeslaus is this still relevant now? It is currently in a state where it up to date enough to pass CI, but there are still some very old (6 years) review comments.

Also, could you take a new look, to tell us if it still shows the patterns we want to show as an example?

It's kind of funny to still see the issue/pr number #20, that is still open. Probably the oldest we have open. Edit: it is the oldest open PR or issue we have

echoix

It is our oldest open PR, number 20. We have to port the docs changes in HTML to markdown. Also, since the last update of the PR, the folder structure changed (in early January 2025, in #4815)

echoix · 2025-02-26T17:23:59Z

Remember to apply the changes in a new markdown file when updating the PR.

echoix · 2025-02-26T17:24:39Z

Remember to apply the changes in a new markdown file when updating the PR.

Abhi-d-gr8 · 2026-02-04T02:46:34Z

Hi! @wenzeslaus and @echoix, I was just seeing the oldest PRs for fun and found that this was stuck on some old merge conflicts and needed the docs converted to Markdown, so I decided to take a look ...

I've gone ahead and rebased this work and converted the docs to Markdown to resolve the conflicts.
To keep things clean, I opened a fresh PR : #7035

Move r.example.segment and r.example.segmulti from doc/raster to doc/examples/raster to follow the directory structure introduced in OSGeo#4815, and adjust the Makefiles and READMEs for the new depth. Add Markdown documentation files as the canonical documentation and keep the HTML files in sync with them. Fix the cross-links in the SEE ALSO sections (review comment), use the current programming manual URL, link the Segment Library page directly, use the map names from the example in the r.example.segment EXAMPLE section, and drop the obsolete CVS date keyword. Use --tmp-project in the READMEs to match the current command line interface. In the example code, process the raster as DCELL regardless of the input map type. Previously, the segment cell size followed the input map type while process() assumed DCELL, so for CELL and FCELL inputs the modification was silently lost (verified by running the tool before and after the change). Free the value buffer in r.example.segmulti (review comment), allocate it with sizeof(DCELL) instead of sizeof(DCELL *), and mark a message for translation. In the library manual page, point the example includes to the new paths, mention Segment_close() in the cleanup step (review comment), show the buffer deallocation in the processing extract, rename the complete example section to match its content (one input raster), and fix typos. Use an absolute path for the doxygen EXAMPLE_PATH so the examples are found both from the top directory and from per-directory runs such as make -C lib/segment htmldox. Update prepared with AI assistance (Claude).

Address review comments on the changed function documentation: In Segment_open(), state that the underlying bytes read as zeros in both the all-in-memory mode (G_calloc) and the temporary file mode (file created by seeking), instead of the incorrect claim that no initialization is guaranteed, while still noting that values are not initialized to raster NULL values. Describe when the all-in-memory mode is used instead of the vague automatic adjustment wording, and attribute the out of memory fatal error to the underlying G_calloc(). Move the comment about the mode marker to the cache allocation because the library detects the all-in-memory mode by the non-NULL cache, not by scb. In Segment_get_row(), extend the description of the stored value size with examples (review comment) and smooth the wording. Update prepared with AI assistance (Claude).

Compile r.example.segment and r.example.segmulti in the step which compiles the other example modules, so the examples stay buildable without being part of the regular build. Update prepared with AI assistance (Claude).

wenzeslaus added 3 commits May 23, 2019 17:48

doc: examples for Segmement Library in dox and as code

52a9359

doc: advanced example for segmentlib in dox from example code

acd56eb

doc: improved desc for open and get_row in segmentlib

08f9480

wenzeslaus added the enhancement New feature or request label May 24, 2019

wenzeslaus requested a review from metzm May 24, 2019 03:22

metzm reviewed May 24, 2019

View reviewed changes

Comment thread lib/segment/segmentlib.dox

\skip G_free

\until Rast_close(output_fd);

metzm May 24, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Segment_close() ?

wenzeslaus added this to the 8.2.0 milestone Dec 2, 2021

wenzeslaus modified the milestones: 8.2.0, 8.4.0 Feb 27, 2022

wenzeslaus modified the milestones: 8.3.0, 8.4.0 Feb 10, 2023

github-actions Bot added C Related code is in C HTML Related code is in HTML libraries docs labels Jun 21, 2024

github-actions Bot reviewed Jun 21, 2024

View reviewed changes

Apply suggestions from clang-format

137e2a1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

echoix changed the title ~~Improve segment library documentation, provide examples~~ man: Improve segment library documentation, provide examples Jun 21, 2024

echoix added 3 commits June 25, 2024 23:26

Merge branch 'main' into segment-lib-doc

0242a7c

Merge branch 'main' into segment-lib-doc

c422f35

Merge branch 'main' into segment-lib-doc

7a641a6

echoix reviewed Dec 21, 2024

View reviewed changes

Comment thread doc/raster/r.example.segment/README.md Outdated

Comment thread doc/raster/r.example.segment/README.md Outdated

Comment thread doc/raster/r.example.segmulti/README.md Outdated

Comment thread doc/raster/r.example.segmulti/README.md Outdated

echoix added 2 commits December 20, 2024 19:22

Add language in markdown fenced code blocks

0feedfc

Merge branch 'main' into segment-lib-doc

7fb1a63

echoix reviewed Dec 21, 2024

View reviewed changes

Comment thread doc/raster/r.example.segmulti/README.md Outdated

Update README.md

a28bf10

echoix reviewed Dec 21, 2024

View reviewed changes

echoix requested changes Feb 26, 2025

View reviewed changes

echoix added the conflicts/needs rebase Rebase to or merge with the latest base branch is needed label Feb 26, 2025

Merge branch 'main' into segment-lib-doc

cc1e0a3

github-actions Bot added the markdown Related to markdown, markdown files label Aug 9, 2025

Merge branch 'main' into segment-lib-doc

9d01154

wenzeslaus modified the milestones: 8.5.0, 8.6.0 Dec 15, 2025

Abhi-d-gr8 mentioned this pull request Feb 4, 2026

doc: Modernize segment library examples and documentation #7035

Closed

wenzeslaus added 4 commits July 16, 2026 09:52

Merge remote-tracking branch 'upstream/main' into segment-lib-doc

ff96095

CI: Compile segment library example modules

090d574

Compile r.example.segment and r.example.segmulti in the step which compiles the other example modules, so the examples stay buildable without being part of the regular build. Update prepared with AI assistance (Claude).

github-actions Bot added the CI Continuous integration label Jul 18, 2026

Uh oh!

Uh oh!

Conversation

wenzeslaus commented May 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metzm May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

metzm May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metzm commented May 24, 2019

Uh oh!

metzm commented May 24, 2019

Uh oh!

mlennert commented May 29, 2019

Uh oh!

wenzeslaus commented Nov 10, 2020

Uh oh!

neteler commented Nov 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

echoix left a comment

Choose a reason for hiding this comment

Uh oh!

echoix left a comment

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!

Abhi-d-gr8 commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

metzm May 24, 2019 •

edited

Loading

metzm May 24, 2019 •

edited

Loading

Abhi-d-gr8 commented Feb 4, 2026 •

edited

Loading