object/function naming

[zackarno]

I thought it would make sense to open a discussion on naming. I have some questions that are not really issues

[zackarno]

Why is the band function named band ? it seems to me like it is essentially a zonal statistic and basically a wrapper for rgee::ee_extract with some nice side effects? Maybe we could consider renaming it? some ideas: reduce_zonal, zonal_stat, zonal_aggregate, zonal_reduce ?

[joshualerickson]

I named it band because of the toBands() function it uses in earth engine prior to doing ee_extract; however, I agree it's misleading. I'd be down to rename, let me think for a sec.

[joshualerickson]

When I think of band I think of time series of some sort. So what do you think of zonal_ts, reduce_zonal_ts. This could also lend itself to changing rr and making it reduce_zonal...

#was rr(), now
get_*() |> reduce_zonal()

#was band(), now
get_*() |> reduce_zonal_ts()

[joshualerickson]

Hey @zackarno, saw your commits and I'll need to take a look but I think that changing the filter functions with what you have would be great. Yeah I'm open to name changes or questions for sure. Just make a PR to main and I'll go through.

[joshualerickson]

Hey just looking at the copied .R files and why don't we do what you recommended and just replace in file instead of making copied version. We'll always have main to fall back on but I think that would be much easier with minimal risk. I've got some time this week to work on the yearly/monthly/year_month filter conversions and look over the band function changes but today i've got to tackle some other stuff. Thanks for your contributions to this!

[zackarno]

Sweet! I will replace the functions in file, remove the copied scipts, and then make a PR. I like the reduce_zonal idea as it can work on both Images and ImageCollections. I think the toBands() part of the original function may not be necessary at this point as rgee::ee_extract works on ImageCollections and can be applied to multiple bands at once. You will see after my PR that I have removed it from band_func and I think this will allow for future flexibility in multi-band reductions (will provide more on this in the PR note).

[zackarno]

What do you think about a common 2-3 letter prefix at least on core functions - like the st_ prefix in the sf package. I find it pretty nice to be able to write in the prefix hit tab and have all the options displayed -> auto-fill. Let me know what you think, it's probably pretty annoying to change function names everywhere, but we could slowly start incorporating this idea if you think it's worth it.

[joshualerickson]

Yeah I like that suggestion. I think we could use ‘ee_’ prefix like ‘rgee’ does. So something like ‘ee_viz’, ‘ee_rr’, ‘ee_filter’, etc. I’ll be working on the PR the next couple days so let me know if ‘ee_’ is good and I can change it. We can just deprecate rr(), viz(), and band(). The new functions will be priority 👍😁

[zackarno]

Cool I can't think of a better prefix than ee_ , but I wonder if it could cause confusion when working with rgee as well. It would be cool if rgee and exploreRGEE functions could always be used together in which case this would not be an issue, but I think there could be an issue due to the get_ object which stores additional information/data structures

[joshualerickson]

I think we could minimize confusion by a using a ‘UseMethod’ call and then you could always use with ‘get_’ and raw ‘rgee’ objects. This would allow coverage over the two classes and also let us feel better about establishing an ‘ee_’ prefix. Only caveat would be the extra args you’d need to apply for it to work, which I don’t think is too big of deal for UX, IMO. Thoughts? Also, I think (as you’ve pointed out) we should continue to move towards this interoperability.

[zackarno]

sweet, I like it - let's go for it