search

R-ArcGIS / arcgislayers

ArcGIS Location Services
http://r.esri.com/arcgislayers/
Apache License 2.0
37 stars 8 forks source link

Fix #166 and issue w/ row-binding results when some elements are empty #167

Closed elipousson closed 1 week ago

elipousson commented 3 months ago

Checklist

Changes

I'll update this description with more detail later this evening.

Issues that this closes

See changes. I'd add more GH issues but my existing reprex is gnarly and I don't know if/when I'll have time to pull together a good one. The arc_read() regression is evident in the existing errored tests – it maybe should get fixed in {arcgisutils} if you can tell what the root source of the problem is.

Follow up tasks

Update README with correct information on handling of filter_geom. That change could easily be added to this PR along with the updates to NEWS.

JosiahParry commented 3 months ago

Thank you!! I've provided some initial thoughts. I'm not all here mentally right now as I'm at our Esri DevSummit to talk about this. :) I'll give it a more thorough review next week.

Regarding the MULTIPOLYGON situation, I might bother the lead dev for the ArcGIS API for Python while I'm here. There may be something else that we could be doing to support it more simply. One option I have in my head is that we can take the sfc and st_cast(filter_geom, "POLYGON") and then run the arc_select() function for each of the polygons and rbinding them. This would make the function recursive though 😨 . Alternatively, we can error and provide a very thorough set of examples in the @details doc that illustrate different techniques for handling it. Because if we can use multipolygons too well, finding a panacea might be a bit tough

elipousson commented 3 months ago

@JosiahParry no rush on my account, I ran into both of these bugs with a project today and figured I'd just get it fixed and open the PR while I had it on my mind. Hope the conference goes well!

Curious to hear what your colleague recommends but I would strongly recommend against having the filter_geom error on MULTIPOLYGON inputs since so sf::st_read() promotes POLYGON geometry up to MULTIPOLYGON by default when reading from a file. The original solution actually worked OK if all the geometries had some overlap. So we could add that back in as a test and use the covex hull method only if it doesn't meet that condition.

JosiahParry commented 3 months ago

@elipousson I have time in the next few days to review this. Do you mind trying to write out what the objective of this PR is? It's a fairly large PR which makes it a bit tough to review!

elipousson commented 3 months ago

It is largely still the same as in the initial description – I ran into a handful of small bugs and found more when I went to fix them. Here are the issues addressed by this PR:

There is also some additional minor refactoring with no change in features that I addressed along the way including adding the rbind_results, rbind_res_list, and match_fields helper functions.

JosiahParry commented 3 months ago

I'm having a fair bit of trouble reviewing this PR because it's so large and broad. This PR is a bit too sweeping to merge at this moment. I think we should break this into smaller more bite-sized pieces that are a bit easier and clearer to tackle. Right now it's all a bit too entwined. I can try and piece apart what the objectives are and then we can start smaller?

Some of the things that this PR does that I think can be broken down into smaller PRs. I'll work on creating issues.

elipousson commented 3 months ago

Thanks for creating all those issues! Totally understand the need to split it up. I need to get a better handle on branching so I can keep developing on my fork and throw things into branches as appropriate.

As is, these mega-PRs are just me fixing things on main as I work through an issue which can be impractical for collaboration. Did you want me to close this PR after I get the more focused ones opened? Or rename this one after pulling out anything beyond the initial scope?

JosiahParry commented 3 months ago

Let's keep this open for now or convert to draft. I think it would be nice to crib from this for each of the PRs.

I think first of all, creating a utility row-binding function would be most immediately useful (selfishly as I'm working on arcgeocode).

What I think would be easiest / most impactful in order:

  1. Row-binding utility (https://github.com/R-ArcGIS/arcgisutils/issues/38#issuecomment-2011051678)
    • including handling empty results

  2. 175

  3. 169

  4. Consistent type checking—i've become a fan of the standalone type checks

  5. 166

elipousson commented 3 months ago

@JosiahParry I have not forgotten about this – just been busy with work. I was able to pull a new branch off from main so I can use that to implement the changes and open a new PR (one at a time unless the changes cover completely separate files) for each of those 5 items.

JosiahParry commented 3 months ago

@elipousson no worries! I'm a bit distracted as well. I've done #1 in https://github.com/R-ArcGIS/arcgisutils/pull/42/files

elipousson commented 1 week ago

Closing this one out since all of the changes are either merged or included with #186