Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r.mapcalc: add data types info to manual #3579

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

pesekon2
Copy link
Contributor

No description provided.

@pesekon2 pesekon2 added manual Documentation related issues raster Related to raster data processing docs labels Apr 11, 2024
@pesekon2 pesekon2 requested a review from ldesousa April 11, 2024 14:51
@pesekon2 pesekon2 self-assigned this Apr 11, 2024
@github-actions github-actions bot added HTML Related code is in HTML module labels Apr 11, 2024
Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

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

Since r.mapcalc has functions to convert data into int, float and double, I agree this is a good place to add such details. I still wonder however if such details should also be in the rasterintro manual instead of the wiki.

raster/r.mapcalc/r.mapcalc.html Outdated Show resolved Hide resolved
raster/r.mapcalc/r.mapcalc.html Outdated Show resolved Hide resolved
raster/r.mapcalc/r.mapcalc.html Outdated Show resolved Hide resolved
raster/r.mapcalc/r.mapcalc.html Outdated Show resolved Hide resolved
raster/r.mapcalc/r.mapcalc.html Outdated Show resolved Hide resolved
pesekon2 and others added 2 commits April 11, 2024 23:05
Suggestions by @veroandreo

Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
@pesekon2
Copy link
Contributor Author

Since r.mapcalc has functions to convert data into int, float and double, I agree this is a good place to add such details. I still wonder however if such details should also be in the rasterintro manual instead of the wiki.

I do agree that it should be added there, too. The wiki information is a well-hidden gem. Do you think we should open a new PR for that one for clarity and cleanness or shall we just stick it to this one (and rename it then)?

Copy link
Contributor

@marisn marisn left a comment

Choose a reason for hiding this comment

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

You'll have to provide better explanation where these values come from. I didn't had time to go over all code, but for me it seems that these numbers are incorrect.
Raster types are defined in gis.h file lines 625–257 as int, float and double. Thus int is a standard C int, that shall be at least -32767 +32767 (see Annex E of ISO/IEC 9899:2017). Ditto for floats and doubles.

Copy link
Contributor

@ldesousa ldesousa left a comment

Choose a reason for hiding this comment

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

OK, this is a tricky one. The Wikipaedia lists the following lengths for these data types:

  • int: minimum 16-bit
  • float: unspecified
  • double: unspecified

The question then is: can GRASS compile on systems where these types have different meanings from those stated? When I started playing with computers most architectures where 16-bit, and by then GRASS was already old. If this is the case, then some reference must be added like "on most systems", or "on 32-bit and 64-bit architectures".

Another thing: would also be important to describe the change in data type length on the int() function itself.

Hat tip @marisn

@marisn
Copy link
Contributor

marisn commented Apr 12, 2024

The question then is: can GRASS compile on systems where these types have different meanings from those stated? When I started playing with computers most architectures where 16-bit, and by then GRASS was already old. If this is the case, then some reference must be added like "on most systems", or "on 32-bit and 64-bit architectures".

Yes it can.
As long as we keep CELL == int, there is a possibility to have any value. One way out would be to define CELL as "as required to keep values in range from MIN to MAX" and then provide magic that would look up suitable data type. Still this would require a lot of changes in the code as many places (unfortunately) are written with CELL == int in mind. As for this PR, I suggest to rewrite it to clearly state that these are limits on an AMD64 (x86_64) architecture when GRASS is compiled with GCC / CLANG (covering most of common systems running GRASS).

@pesekon2
Copy link
Contributor Author

@marisn: Thanks for the thoroughful info. The docs were updated in 08aa852, trying to address the issue.

@marisn marisn merged commit 79fb976 into OSGeo:main Apr 17, 2024
23 checks passed
@neteler neteler added this to the 8.4.0 milestone Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs HTML Related code is in HTML manual Documentation related issues module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants