Skip to content
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

Make StlResults fields public #6

Closed
wants to merge 1 commit into from
Closed

Conversation

sd2k
Copy link
Contributor

@sd2k sd2k commented Sep 9, 2023

It's currently hard to store the results of the STL decomposition without using to_vec() on the various slices returned by the seasonal/trend/remainder/weights accessor methods, which unfortunately requires cloning all the data. By making the fields public the caller can decide what to do with them.

An alternative could be to add some sort of into_parts() method which returns all four as a tuple or something, but this just seemed easier!

It's currently hard to store the results of the STL without
using `to_vec()` on the various slices returned by the
seasonal/trend/remainder/weights accessor methods, which
unfortunately requires cloning all the data. By making the
fields public the caller can decide what to do with them.

An alternative could be to add some sort of `into_parts()`
method which returns all four as a tuple or something, but
this just seemed easier!
@ankane
Copy link
Owner

ankane commented Sep 18, 2023

Hi @sd2k, thanks for another PR! The methods will return slices in 0.3.0, which won't require cloning, so this shouldn't be needed.

@ankane ankane closed this Sep 18, 2023
@sd2k
Copy link
Contributor Author

sd2k commented Sep 18, 2023

@ankane that does sound like a good step but my issue is that I want to take ownership of one or two of the individual components, moving them out of StlResults and into a custom struct. I won't be able to do that without either a) making the Vec fields public, b) adding some other method to take all the components out, or c) cloning the slices 😞

@sd2k
Copy link
Contributor Author

sd2k commented Sep 18, 2023

Here's a more concrete example of where I'm doing that in my fork: I'm implementing MSTL which iterates a few times and calculates the most recent seasonal component for a few seasonal lengths. Without ownership of the Vec I'd need to store either the whole StlResults field in the hashmap (which is unneeded, I just need the seasonal part) or clone the seasonal component on each iteration.

@ankane
Copy link
Owner

ankane commented Sep 18, 2023

Thanks for the additional context. Cloning the data should be fine for this, as it'll be extremely fast compared to running STL.

ankane added a commit that referenced this pull request Sep 22, 2023
@ankane
Copy link
Owner

ankane commented Sep 22, 2023

Hi @sd2k, thinking about this more, I think an into_parts function makes sense, so added in the commit above.

I also read the MSTL paper and am planning to include it in 0.3.0.

@ankane
Copy link
Owner

ankane commented Sep 22, 2023

Thanks for the suggestion, and sorry for the initial hesitation.

@sd2k
Copy link
Contributor Author

sd2k commented Sep 22, 2023

@ankane Nice, that's great to hear! No worries re. hesitation, it's much harder to remove something once it's been added so I understand you wanted to consider it 🙂

@ankane
Copy link
Owner

ankane commented Sep 22, 2023

Added initial support for MSTL in 33ed566

@ankane
Copy link
Owner

ankane commented Sep 22, 2023

Added more parameters in follow-up commits. Let me know if you have any feedback and will cut a new release.

@sd2k
Copy link
Contributor Author

sd2k commented Sep 22, 2023

Oh nice! I wasn't expecting MSTL to be added to this crate, was happy implementing it myself, but it's good to see a second implementation.

I'll try to adapt augurs to use your MSTL impl to check that the APIs all work fine, but it might be a couple of days before I get to it. Thanks again!

@sd2k sd2k mentioned this pull request Sep 23, 2023
@sd2k
Copy link
Contributor Author

sd2k commented Sep 23, 2023

I filed #7 to add a few more derived traits but other than that everything works nicely! grafana/augurs#37 incorporates your MSTL support into augurs 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants