-
Notifications
You must be signed in to change notification settings - Fork 4
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
ee_closest_distance_to_val #17
base: main
Are you sure you want to change the base?
Conversation
…l` with vignette
…up w/ discussion on this
Hey Zach, let me think on this one a little bit. I'm taking a look and getting an idea of what you're looking at doing (looks really cool). It reminds me of a 'get_*' function but with other applications so I'm more inclined to keep the naming convention and args. I'll makes some comments and get back with you soon. Thanks for doing this, looks sweet! As to the ee_timeseries idea, I'm not sure that we need to change it too much. I could see removing the filter args so that the user has to pre-process. Ok, wait. I do like that idea... So if we take out the temporal args and replace with ee_extract() then it would essentially be a
The cons would be losing the sugar. For example, same thing but removes the need to 'pre-process'. With
I'm not sure how I feel about this. We might consider something like I can work on this and review the PR. Might be a week or so but stay in touch. Thanks! |
Cool, yeah i think there is no harm in separating the filter/pre-process and the extract steps – then we will inevitably build a wrapper to combine them again which will allow the sugar, but end up with nicer code i think. I think However, now thinking about it, I think I have two ideas which might be good. The first one would be a pretty quick fix and basically builds upon what you are proposing by splitting Idea 1:
Option 2:
We could potentially implement the idea 1 first and then transition to idea 2 if we think it's good or just skip to 2. It might be a lot of work. It is kind of interesting because if we go with idea 1 -> idea 2 the work flow kind of mirrors what happened with the survey package which had |
I like option 2! So just create methods that handle image/imageCollection classes for
I think it's worth working on this and establishing this type of style. I'll review your |
Yes, methods for |
Ok, so a lightweight package {rgeeTidy} or {tidyRGEE} with S3 methods for tidying image and imageCollections that we can then use for {exploreRGEE} or {easyrgee}. Sounds good to me! I'll leave this PR open until we get that implemented and then we can just tie it into the code. Might make sense to chat again about direction as well. I'm thinking it's getting more relevant to separate {exploreRGEE} and focus on {easyrgee} or from scratch? Let me know what you think. Did you want to get this going? I've got some time and don't mind but if you want to do it then I'd say go for it :). Have a good one! |
Yeah, i think that makes sense. I'll email to see if we can arrange a chat! |
I added function to calculate the closest distance between any point geom and specified pixel value (image/imageCollection). There is an
rmd
which i think explains it better.ee_timeseries
with a new method to allow it takeee$Image
as well asee$ImageCollection
. I did this hacky work around so that there would be minimal changes to the code in order limit downstream code breakage. This works for now, but it makes the syntax of using theimageCol
parameter a little murky since we can also use just images.map_date_to_bandname
to allow imageI think it might be worth thinking about the syntax on
ee_timeseries
. Perhaps it could work to just copy the syntax used inrgee::ee_extract
- this might clear things up and improve compatibility. As for the name, i think it's fine - if you provide an image you just end up with 1 point in time. However, I could see a potential argument for changing it.As for the function itself, I am still considering it a bit experimental - i think it is working, but i need to test it more. What do you think about merging this type of function to the
main
with this caveat?