search

ncss-tech / aqp

Algorithms for Quantitative Pedology
http://ncss-tech.github.io/aqp/
54 stars 15 forks source link

Remove `"original.order"` from SPC metadata #310

Closed brownag closed 6 months ago

brownag commented 6 months ago

This was only used for reorderHorizons() default functionality. It is rare that this function is used, nor that we don't know the desired target order. reorderHorizons() now requires target.order be specified

Metadata with a numeric vector of the order of input data was set for every SPC (which could be a bit costly in terms of time/memory for large SPCs) but only was used for reorderHorizons() which is rarely needed or used.

This metadata entry, and the formerly present target.order metadata, were held over from my ideas on how to implement alternate profile sorting methods (i.e. not dependent on alpha sorting of profile ID + top depth). We decided to not change the historic sorting behavior.

I think that the better way to implement numeric/alternate profile sorting in the SPC--if we ever want to--would be to have some sort of an internal numeric profile ID assigned at object creation (i.e. as.integer(factor(horizons(x)[[idname(x)]]))) rather than relying on profile ID (character) sorting.

dylanbeaudette commented 6 months ago

Sounds good. We can re-visit profile sorting after this flurry of PRs. While I originally was very interested in explicit profile re-ordering, there have been very few cases where I've needed to do it over the years. The on-the-fly sorting in plotSPC() is usually enough for my needs. Still, would be useful down the road.

brownag commented 6 months ago

there have been very few cases where I've needed to do it over the years.

I think (or rather know) people find the alpha sorting of numeric profile ID values, in particular, to be confusing. It came up in recent STAT1010 offering.

It always bothers me to see it in show output for fetchNASIS()/KSSL type SPCs, but I have gotten used to it. I generally agree there is little need to actually reorder profiles for any particular analyses-- but most people don't expect numbers to be sorted that way and it makes them wonder if something is wrong.

brownag commented 6 months ago

We can re-visit profile sorting after this flurry of PRs.

Also, I want to add, I am not specifically advocating for that. I just was explaining why this metadata entry existed in the first place. Custom sorting profiles is a complex issue, if it could be done simply without changing a lot of things I probably would have submitted a PR for it a while ago. IMO there is much more higher priority stuff in the issue tracker that we need to solve before introducing anything like that.

No rush on my end to get any of these PRs in, I just had a stack of stuff to put out there in the ether. It is probably good to hold the big / breaking changes until we are ready to release v2.1... which, with STAT2020 next week--should probably be at least 3 weeks out or more due to breaking changes in the SPC object.

This metadata one is not a big change and probably won't affect anyone, but since it is related to SPC internals may be good to hold off on anyway. As far as the adding new function PRs those do not need to be merged at all, I just accumulate a lot of scratch functions when trying to do other things, so me creating the PRs is just a way of putting it out for feedback, checking documentation, generalizing. I often find that process reveals issues or unexpected ideas.