func - random_spatial_sample

zackarno commented 6 months ago

add func to drop points in any poly. Can be used for to generate points for proportional bubble map. Right now it just takes an sf class poly as an argument.

https://github.com/OCHA-DAP/hdx-signals/issues/65#issue-2287988570

Curious if you also get this warning below when pushing. I don't know what I could have done to generate it because I THINK i just edited this map_test.R file

remote: Resolving deltas: 100% (652/652), done.
remote: warning: See https://gh.io/lfs for more information.
remote: warning: File e4cce236d9b86bd566656f9318638d78aa3639d8 is 93.86 MB; this is larger than GitHub's recommended maximum file size of 50.00 MB
remote: warning: GH001: Large files detected. You may want to try Git Large File Storage - https://git-lfs.github.com

caldwellst commented 6 months ago

Lastly, I think the point generation should be added to the map_test() function.

zackarno commented 6 months ago

thanks for all the comments! working on integrating them and am sure will have various replies comments as I go, but real quick regarding the style comments - how do we run styler on this repo since it's not included as a dep in {renv} -- I'd like to just run that and have it take care of alot of the style inconsistencies you spotted

zackarno commented 6 months ago

Lastly, I think the point generation should be added to the map_test() function.

On this, I wasn't entirely sure what you would done so didn't want to disrupt the flow. But I was thinking it could be beneficial to unwrap some of the nested/wrapped functionality inside map_test to make it more modular and easier for little developments (i.e adding point generation to maps). I was thinking we shouldn't have the load_* functionality built into the geom() functions. As this means we have to load admin boundary once to map it and then load it again just sample it. Feel like it would be good to continue (as in the rest of repo) to have functions more split by processes... for example : 1. loading, 2. processing, 3. visualizing/plotting, 4. saving. That way it's easier to check each phase when developing or manually testing.

Idk if what I say here makes sense, but I didn't want to start deconstructing any functions too much until we chatted so lets discuss tomorrow and figure out what makes most sense

caldwellst commented 6 months ago

So, map_test() is a convenience function used to test mapping conventions in terms of legend placement and saving dimensions, so one of our manual testing implementations. So not as worried about some of the efficiency things since the utilities are designed for use in the runs as well.

was thinking we shouldn't have the load_* functionality built into the geom() functions.

So, I would agree if we were building for map_test() alone, but I think the convenience is that you can just have a single geom() call to add in the things you want in all maps: adm0 boundaries and cities, and we aren't going to be messing with those ideally. Otherwise, in each map function for the indicators, we have to individually load in then map those.

how do we run styler on this repo since it's not included as a dep in {renv}

Simply install.packages("renv")! You can install whatever you want, and since you aren't using it in a script, it won't be caught by snapshot() by default and won't be in the lock file. It's how I often use tools like {usethis} in repos that use {renv}. As well, #79 is discussing using pre-commit which can automatically lint and maybe even style the code.

zackarno commented 6 months ago

ok so I re-worked it to integrate the latest tip of main as there were some changes to the map implementation that was affecting some of the aesthetics.
The functions are all in map_test.R below your original function map_test() which I think should be deleted, just want to confirm.
Then I've added a script where implement the main function from map_test.R to generate all the maps. It's in tests/manual/implement_test_maps.R. I don't think this where you would have put it and i'm guessing it won't jive well with the testthat work flow.. anyways I put it in there for now so you can clarify.
Based on the test email you sent out the aesthetics of the embedded map do look quite a bit different than the output pngs (different size text is the most notable), therefore I had thought you must have added some code to adjust this this, which is why I went through the trouble of merging the latest tip of main into this branch. However, as you noted the tweaks are only minor, they do change the appearance to a degree, but don't explain all the differences I see in the email test and the pngs... Any idea what could it be causing this? It could be something I missed in the code, some mailchimp resizing, or ... something ?
You can test it out by running the script here: tests/manual/implement_test_maps.R, just change the out_dir argument to something that makes sense to you.
Note I changed 2 argument names in map_points.R ... maybe not perfect yet, but old ones were confusing me as well
- val_col -> pt_value_name: since the map_points() func actually plots the entire basemap and points I thought it was good to specify which geom are talking about. Also col sometimes means color especially in the context of plotting
- size - > pt_value_label- this param just supplies the legend label, so i thought it made more sense.

zackarno commented 6 months ago

since PR is still open I figured I'd add the assert_bbox_overlap() function to the PR.
- i just put it in a new file called src/utils/assertions.R but am happy to reorganize as not sure where you would want it.
- Also was curious on the plan for unit-testing, are we going to use {testthat}? Was eager to add unit-tests for this functions just simply because I had made like >90% of a test right in the roxygen so would be super easy to just add. I know there is a cool package for creating unit tests directly from roxygen. I think it's called {doctest}. Anyways, maybe we should start a discussion thread if lots of decisions to be made on this.

caldwellst commented 6 months ago

For testing, feel free to add. Don't think is critical because this is a test function, though, just something we want to manually run to check maps are looking alright. Not sure what is causing the text differences to be honest!

zackarno commented 6 months ago

Thanks for the comments -- all super useful and simplifying! I think i've integrated them all so ready for another review.

re-testing, yeah it's not critical I was just curious on the overall plan for unit-tests because as everyone always says... its way easier if you just write them as you go. Although I've never been strict enough with myself to do this, I thought i'd give it a try since I basically had the test ready to go

zackarno commented 6 months ago

o woops let me move the files around to the new directory first tests/manual/maps first

zackarno commented 6 months ago

ok now should be good to go!

zackarno commented 6 months ago

Not sure what is causing the text differences to be honest!

This seems critical. Hope it's not the same issue I was getting into w/ tmap on nga flood pilot as that was never resolved and made visual testing much more difficult

caldwellst commented 6 months ago

lol we have complete differences in what is being generated on our machines... @hannahker can you run this code and output it to another folder on our Google Drive, next to tmp/seth_map_test and Zack's dated folder?

zackarno commented 6 months ago

lol we have complete differences in what is being generated on our machines... @hannahker can you run this code and output it to another folder on our Google Drive, next to tmp/seth_map_test and Zack's dated folder?

lol thats concerning!

@hannahker fyi its just the script in test/manual/maps/implement_map_test.R

hannahker commented 6 months ago

Just ran -- see outputs under hannah_map_test. My outputs look pretty similar to @caldwellst's. I think it would make sense to test on an Actions runner too, since this is where they'll be generated in production?

caldwellst commented 6 months ago

Yes, agreed, see #90 for my idea of how we get to where this is easily possible. Something for next week.

OCHA-DAP / hdx-signals

func - random_spatial_sample #75