Fixing Apple Silicon issues, getting rid of dplyr, rlang and sp dependencies, adding collapse and s2 = more lightweight and multiple times faster grid materialization, and some small convenience features. #66

SebKrantz · 2023-07-07T04:01:02Z

No description provided.

…e optimization

… of a double (factor) to long int here: on M1 Macs this is rounded downwards, thus I needed to add a small number. Took me 6 freaking hours of simultaneous debugging on Mac and Windows to find it...

…at frame (can be converted to S2 and other classes).

SebKrantz · 2023-07-15T21:44:37Z

@r-barnes, first of all thanks for making such an awesome software available for R. I have implemented some fixes, especially for Mac users (#63), made the package more lightweight and significantly faster, and added some vital convenience features. Please consider merging this PR and pushing out a new version to CRAN. See the NEWS file for a summary of the changes. Thanks!

jsocolar · 2023-10-17T20:37:29Z

Just floating back here to thank @SebKrantz for the fix and also to ask if there is hope of getting this merged and pushed up to CRAN soon. As things stand, I believe M1 users can still get the wrong behavior silently, which is worth patching with some urgency IMO.

r-barnes · 2023-10-18T11:12:03Z

src/DgHexIDGG.cpp

@@ -167,7 +167,7 @@ DgHexIDGG::initialize (void)
      if (isClassIII())
         factor *= M_SQRT7;

-      maxD_ = factor - 1.0;
+      maxD_ = factor+0.000001 - 1.0;


Why is this being added? Can you rewrite as 1e-6?

Add a comment before the line explaining the offset.

src/shputils.c

r-barnes · 2023-10-18T11:14:32Z

src/DgTriIDGG.cpp

@@ -117,7 +117,7 @@ DgTriIDGG::initialize (void)
      double factor = parentScaleFac * 2.0L; // aperture 4

      scaleFac_ = factor;
-      maxD_ = factor - 1.0L;
+      maxD_ = factor+0.000001 - 1.0L;


Use 1e-6 add a comment before the line explaining the offset.

r-barnes · 2023-10-18T11:14:42Z

src/DgHexIDGG.cpp

@@ -167,7 +167,7 @@ DgHexIDGG::initialize (void)
      if (isClassIII())
         factor *= M_SQRT7;

-      maxD_ = factor - 1.0;
+      maxD_ = factor+0.000001 - 1.0;


Add a comment before the line explaining the offset.

r-barnes · 2023-10-18T11:25:09Z

@SebKrantz - Let's get this merged.

One complicating factor is that any changes made to the C++ code need to be upstreamed to https://github.com/sahrk/DGGRID, so I need to be able to justify and explain them to @sahrk. I'm not sure why you've added factor + 1e-6 - can you add a comment explaining that?

I've just updated this file to explain the C++ workflow. If you'd like, I can handle that for this PR.

SebKrantz · 2023-10-18T11:25:32Z

@r-barnes I have used scientific notation and added a comment: the apple ARM issue was that conversion to integer rounded the number downwards which caused a lot of problems down the line. Adding a small number changed this. This has probably something to do with ARM systems not supporting long double. I note that @sahrk has also merged my PR in the DGGRID C++ library. .

r-barnes · 2023-10-18T11:26:23Z

src/shputils.c

            if (dlow < dhigh)       dgprintf(stmp,dlow,dhigh,mean);
            else if (dlow == dhigh) {
-                sprintf(stmp,"= %%.%df",iDecimals);
+                snprintf(stmp, sizeof(stmp), "= %%.%df",iDecimals);


This needs to be dgprintf

Ok, is that a wrapper around snprintf that works the same? R does not allow calls to sprintf anymore.

r-barnes · 2023-10-18T11:29:04Z

@SebKrantz - Oh, I see you've already got the fix upstreamed - nice!

r-barnes · 2023-10-18T11:38:30Z

@SebKrantz - The latest upstream pull from DGGRID will take me a little bit to verify. I'm working on it.

SebKrantz · 2023-11-12T01:51:11Z

@r-barnes any news here? Does there need to be an upstream pull? In my experience the patch is working perfectly.

SebKrantz · 2024-03-15T22:18:44Z

@r-barnes, I‘ve now set up my fork so that people can easily find and install from there. But needless to say this is not ideal. I would be happy to become the CRAN maintainer of dggridR and push this out to CRAN if you feel like this is something you can no longer do. Let me know.

r-barnes · 2024-03-15T22:25:47Z

Sebastian, I'm happy to add you to the maintainers list, though I will have to walk to you through the process of bringing in upstream DGGRID. Maybe we can find time to set up a call?

…

On Fri, Mar 15, 2024, 15:19 Sebastian Krantz ***@***.***> wrote: @r-barnes <https://github.com/r-barnes>, I‘ve now set up my fork so that people can easily find and install from there. But needless to say this is not ideal. I would be happy to become the CRAN maintainer of dggridR and push this out to CRAN if you feel like this is something you can no longer do. Let me know. — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXZHVBZ6ZAOICPR5R3PCVLYYNXVXAVCNFSM6AAAAAA2BITKL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQGU3TMNRRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

SebKrantz · 2024-03-15T22:34:25Z

@r-barnes Thanks, happy to set up a call after 22nd March (on Conference Travel this week). However, as indicated, I don’t see the need for an upstream pull, as the package works perfectly well as it is now. Maybe I‘m missing something, but I see the upstream pull rather as something that will just delay deployment to CRAN and might introduce further issues. But yeah, happy to chat about it. Perhaps just send me a meeting suggestion to [email protected]

AlbanSagouis · 2024-05-02T13:23:47Z

Beautiful work @r-barnes, thank you!
@SebKrantz, thank you so much, your fork immediately solved my problem.

SebKrantz · 2024-05-05T19:07:50Z

@r-barnes what would speak against merging this PR and sending it off to CRAN? (I'd adjust the README again). #70 also suggests that updating C++ may be complicated. Anyway, I'd be available for chats about maintenance this month.

edzer · 2024-05-17T15:31:51Z

If you consider changing maintainer, one could consider to migrate this package to the r-spatial GH organisation, where it maybe receives more eyes; I'd be happy to invite everyone here as contributer there. I'm planning to spend time this fall on problems around grids crossing the antemeridian, as reported e.g. in r-spatial/sf#280

r-barnes · 2024-05-20T21:35:19Z

@SebKrantz - I think the only part of this PR that's challenging are the changes to DGGRID. If you're able to split those off, I think I can land everything else immediately.

r-barnes · 2024-05-20T21:41:03Z

@SebKrantz @edzer - I'm generally pro adding maintainers and I'd appreciate the help. I'm worried about diverging too far from upstream DGGRID in case there are bug fixes or such available from it.

SebKrantz · 2024-05-20T22:18:56Z

@r-barnes of course the C++ changes could be split off, but that then means that we publish another version of the software that will not work on Mac and any other system that misses the long double type. The C++ changes are minor and have been upstreamed to DGGRID, but if you want to do a full C++ update instead I would suggest to build my fork and send that to CRAN and then do the C++ update. I can then remove the C++ changes from the master branch of my fork for you to merge it.

r-barnes · 2024-05-21T13:57:40Z

@SebKrantz - if the changes have all landed upstream, then we can do the merge here.

r-barnes · 2024-05-21T14:00:44Z

src/DgHexIDGG.cpp

-
-      maxD_ = factor - 1.0;
+      // Adding small number (1e-6) to prevent rounding down in conversion to integer (fixes issue #63 experienced on Apple ARM computers)
+      maxD_ = factor+1e-6 - 1.0;


std::nextafter might have been a mildly safer way to do this.

r-barnes · 2024-05-21T14:01:15Z

R/dggridR.R

+    layer <- file_path_sans_ext(basename(shpfname))
+    bb  <- st_bbox_to_sp(st_bbox(st_read(dsn, layer=layer)))
+
+  } else bb <- st_bbox_to_sp(st_bbox(shpfname))


This would do better in brackets.

r-barnes · 2024-05-21T14:02:02Z

@SebKrantz - I've merged having a left a couple of comments.

SebKrantz · 2024-05-21T14:07:28Z

@r-barnes excellent! You'll have to adjust the README.md again then.

r-barnes · 2024-05-21T14:08:47Z

@SebKrantz - Shoot, I should have squash-merged that. Just did a local rebase and force-pushed.

r-barnes · 2024-05-21T14:10:25Z

@SebKrantz - Good catch on the readme.

r-barnes · 2024-05-21T14:16:42Z

@SebKrantz and @edzer - I've given you collaborator access to the repo, set master to require a code review, and setup Squash+Merge as the available merge type.

SebKrantz · 2024-05-21T14:28:14Z

Thanks @r-barnes. I suppose you'll prepare a CRAN release? I'd also support @edzer's suggestion to move this to r-spatial for more visibility, but up to you of course.

SebKrantz added 6 commits July 7, 2023 03:41

Getting rid of dplyr and sp: replacing with collapse, sf and s2 + som…

97375b5

…e optimization

Small fix.

0280fc7

Need to add sf and rename count column.

d26aab9

Using R's numeric() instead of alloc().

25ae164

Update NEWS

682fec8

Fixing r-barnes#63. The issue on M1 Mac is due to implicit conversion…

1a6c41b

… of a double (factor) to long int here: on M1 Macs this is rounded downwards, thus I needed to add a small number. Took me 6 freaking hours of simultaneous debugging on Mac and Windows to find it...

SebKrantz added 2 commits July 15, 2023 23:36

Adding return_sf argument: set to FALSE for memory efficient long dat…

2b9fb84

…at frame (can be converted to S2 and other classes).

Appeasing R CMD check.

1363907

SebKrantz mentioned this pull request Jul 15, 2023

FATAL ERROR: DgQ2DDtoIConverter::convertTypedAddress() #63

Closed

r-barnes requested changes Oct 18, 2023

View reviewed changes

Comment and change of notation as requested by @r-barnes.

086dac1

r-barnes requested changes Oct 18, 2023

View reviewed changes

SebKrantz added 2 commits October 18, 2023 13:33

Using dgprintf as requested by @r-barnes.

d74597e

Merge branch 'master' into master

6b5856e

Also using dgprintf here.

87f5e10

SebKrantz added 3 commits March 10, 2024 19:20

Install instructions for fork.

b72aef1

Update README.md

5405086

Remove magrittr pipe.

1a7dce3

Need to add dplyr for vignette it seems.

f5bb0df

r-barnes approved these changes May 21, 2024

View reviewed changes

r-barnes merged commit 4d59137 into r-barnes:master May 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Apple Silicon issues, getting rid of dplyr, rlang and sp dependencies, adding collapse and s2 = more lightweight and multiple times faster grid materialization, and some small convenience features. #66

Fixing Apple Silicon issues, getting rid of dplyr, rlang and sp dependencies, adding collapse and s2 = more lightweight and multiple times faster grid materialization, and some small convenience features. #66

SebKrantz commented Jul 7, 2023

SebKrantz commented Jul 15, 2023

jsocolar commented Oct 17, 2023

r-barnes Oct 18, 2023

r-barnes Oct 18, 2023

r-barnes Oct 18, 2023

r-barnes Oct 18, 2023

r-barnes commented Oct 18, 2023

SebKrantz commented Oct 18, 2023 •

edited

Loading

r-barnes Oct 18, 2023

SebKrantz Oct 18, 2023

r-barnes commented Oct 18, 2023

r-barnes commented Oct 18, 2023

SebKrantz commented Nov 12, 2023

SebKrantz commented Mar 15, 2024

r-barnes commented Mar 15, 2024 via email

SebKrantz commented Mar 15, 2024 •

edited

Loading

AlbanSagouis commented May 2, 2024

SebKrantz commented May 5, 2024 •

edited

Loading

edzer commented May 17, 2024

r-barnes commented May 20, 2024

r-barnes commented May 20, 2024

SebKrantz commented May 20, 2024

r-barnes commented May 21, 2024

r-barnes May 21, 2024

r-barnes May 21, 2024

r-barnes commented May 21, 2024

SebKrantz commented May 21, 2024

r-barnes commented May 21, 2024

r-barnes commented May 21, 2024

r-barnes commented May 21, 2024

SebKrantz commented May 21, 2024

Fixing Apple Silicon issues, getting rid of dplyr, rlang and sp dependencies, adding collapse and s2 = more lightweight and multiple times faster grid materialization, and some small convenience features. #66

Fixing Apple Silicon issues, getting rid of dplyr, rlang and sp dependencies, adding collapse and s2 = more lightweight and multiple times faster grid materialization, and some small convenience features. #66

Conversation

SebKrantz commented Jul 7, 2023

SebKrantz commented Jul 15, 2023

jsocolar commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-barnes commented Oct 18, 2023

SebKrantz commented Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-barnes commented Oct 18, 2023

r-barnes commented Oct 18, 2023

SebKrantz commented Nov 12, 2023

SebKrantz commented Mar 15, 2024

r-barnes commented Mar 15, 2024 via email

SebKrantz commented Mar 15, 2024 • edited Loading

AlbanSagouis commented May 2, 2024

SebKrantz commented May 5, 2024 • edited Loading

edzer commented May 17, 2024

r-barnes commented May 20, 2024

r-barnes commented May 20, 2024

SebKrantz commented May 20, 2024

r-barnes commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-barnes commented May 21, 2024

SebKrantz commented May 21, 2024

r-barnes commented May 21, 2024

r-barnes commented May 21, 2024

r-barnes commented May 21, 2024

SebKrantz commented May 21, 2024

SebKrantz commented Oct 18, 2023 •

edited

Loading

SebKrantz commented Mar 15, 2024 •

edited

Loading

SebKrantz commented May 5, 2024 •

edited

Loading