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

Implementing the Lambert W function? #64

Closed
JSorngard opened this issue Jul 29, 2024 · 10 comments
Closed

Implementing the Lambert W function? #64

JSorngard opened this issue Jul 29, 2024 · 10 comments

Comments

@JSorngard
Copy link
Contributor

JSorngard commented Jul 29, 2024

Hi!

Thank you for this nice crate!
I recently finished an implementation of the Lambert W function using the fast and accurate method of Toshio Fukushima. Would it be interesting to integrate this to the special function capabilities of this crate?

@Axect
Copy link
Owner

Axect commented Jul 29, 2024

Hello @JSorngard,

Thank you for your interest in Puruspe and for your excellent suggestion! I've had a chance to look at your Lambert W function implementation in your GitHub repository, and I'm truly impressed. Your clean, dependency-free Rust code aligns perfectly with Puruspe's philosophy.

I'm very excited about the possibility of integrating your implementation of the Lambert W function into puruspe. The use of Toshio Fukushima's fast and accurate method sounds like it would be a great addition to our special function capabilities.

I'd be delighted if you could submit a pull request with your implementation. Feel free to include any tests or documentation you think would be helpful. If you need any assistance with the integration process or have any questions about our project structure, please don't hesitate to ask.

Once again, thank you for your valuable contribution offer. Your work could significantly enhance puruspe's functionality, and I'm looking forward to the possibility of including it.

Best regards,
Axect

@JSorngard
Copy link
Contributor Author

JSorngard commented Jul 30, 2024

Thank you for your comment!

I just have a few questions:

Would it make sense to just use lambert_w as a dependency in peroxide?

If not I'll make a fork and start integrating parts of it here during the coming days, but then I have two more questions:

  • Which version of the functions should I integrate? (I would suggest the most accurate ones as they are still very fast).
  • Currently they return an Option which is None outside the domain of the function. Should I change this to return NAN or something else?

@Axect
Copy link
Owner

Axect commented Jul 30, 2024

Thank you for your questions. I appreciate your thoughtfulness in approaching this integration. Let me address each of your points:

  1. Regarding using lambert_w as a dependency:
    For Puruspe, we aim for no dependencies, so integrating directly into the codebase would be preferable. However, for Peroxide, using lambert_w as a dependency is perfectly acceptable as long as it's pure Rust.

  2. Integration approach:
    For Peroxide, I suggest we use lambert_w as a dependency and create a consistent wrapper. The wrapper would follow the pattern in https://github.com/Axect/Peroxide/blob/master/src/special/function.rs, with functions taking and returning f64 values.

  3. Error handling:
    For consistency with other Peroxide functions, I suggest the wrapper return NaN for out-of-domain inputs, unless there's a critical reason not to do so.

  4. Version selection:
    Peroxide is structured with two main modules: fuga for full customization and prelude for convenience. This structure aligns well with the two versions of Lambert W functions you've implemented.

    For fuga, we can provide full flexibility:

    #[derive(Debug, Copy, Clone)]
    pub enum LambertWPrecision {
        Simple,
        Precise,
    }
    
    pub fn lambert_w_0(z: f64, precision: LambertWPrecision) -> f64 { // implementation }
    pub fn lambert_w_m1(z: f64, precision: LambertWPrecision) -> f64 { // implementation }

    This allows users to choose between the 24-bit (Simple) and 50-bit (Precise) versions.

    For prelude, we can default to the faster, simpler version:

    pub fn lambert_w_0(z: f64) -> f64 { // calls the simple version }
    pub fn lambert_w_m1(z: f64) -> f64 { // calls the simple version }

    This approach provides the flexibility needed in fuga for advanced users who may need higher precision, while maintaining simplicity in prelude for common use cases.

What do you think about aligning the Lambert W functions with Peroxide's existing structure in this way? Feel free to suggest any modifications to the names or structure - these are just initial ideas to get us started.

@JSorngard
Copy link
Contributor Author

JSorngard commented Jul 30, 2024

Thank you for the feedback!

I have a question about implementing two different versions, one in prelude and one in fuga. Currently both modules do

pub use crate::special::function::*;

This means that whatever function is implemented in the function module will be exported to both prelude and fuga.

I can think of a five approaches:

  1. Implement the non-flexible version in function.rs and change the import in fuga to be all functions except Lambert W, and then implement a flexible version in fuga/mod.rs.
  2. Implement the flexible version in function.rs and change the imports in prelude/mod.rs to be all functions except Lambert W, and then implement a non-flexible version in prelude.
  3. Implement one version in function.rs and also the other version in the appropriate module.
  4. Implement one version in function.rs and let that be the version in both modules.
  5. Implement the functions only in prelude and fuga, and not in function.

None of these feel perfect to me.

@Axect
Copy link
Owner

Axect commented Jul 30, 2024

Thank you for your insightful question about the implementation structure. Your attention to the architectural details is much appreciated. You've raised a valid point about the current import strategy in prelude and fuga. Let me provide some context and suggest a path forward:

  1. The reason we've been using pub use crate::special::function::* in both prelude and fuga is that, until now, we haven't had functions in special::function that provide customizable options. Your Lambert W implementation is introducing this new scenario.

  2. When we typically implement functions with different levels of flexibility for prelude and fuga, our usual approach is:

    • Implement the flexible version as the base implementation.
    • Create corresponding non-flexible functions in prelude/simpler.rs.
    • Import these simpler functions into prelude.

This approach has worked well for us in maintaining both flexibility and simplicity where needed.

Given this context, here's what I propose for the Lambert W implementation:

  1. Implement the flexible version (with the LambertWPrecision enum) in special/function.rs.
  2. In prelude/simpler.rs, implement wrapper functions that use the 'Simple' precision by default.
  3. Update prelude/mod.rs to import the simpler Lambert W functions from simpler.rs, while still importing everything else from special::function.

This way, fuga users get the flexible version, while prelude users get the simpler version.

However, I want to emphasize that this is just a suggestion based on our current structure. If you feel this approach is overly complex for your implementation, I'm completely open to your original proposal of implementing just the 24-bit precision version, which offers a good balance of accuracy and speed. This simpler approach would work well as a wrapper in special/function.rs.

What are your thoughts on these options? Feel free to choose the approach you're most comfortable with, or suggest any modifications. The goal is to integrate your excellent work in a way that fits well with Peroxide's structure and is maintainable for you.

@JSorngard
Copy link
Contributor Author

JSorngard commented Jul 30, 2024

Ah, I didn't see the simpler.rs file! Yes that makes a lot of sense.

The only additional suggestion I have then is to use the accurate versions in prelude, as getting accurate results is probably what the user expects, and then an advanced user can use the flexible version to sacrifice the accuracy for speed if they really need it.

The speed difference between the 24 and 50 bit versions (when benchmarked on my machine) are around 15 to 25%, but they both take on the order of single digit nanoseconds to execute.

@Axect
Copy link
Owner

Axect commented Jul 30, 2024

Thank you for your feedback and additional suggestion. I completely agree with your perspective.

You're absolutely right - using the more accurate 50-bit version in prelude makes perfect sense. As you pointed out, users typically expect the most accurate results by default, and the performance difference is minimal in most cases. This approach aligns well with our goal of providing the best out-of-the-box experience for users.

If you're happy with this plan, feel free to proceed with the implementation. If you have any further thoughts or questions, please don't hesitate to share them.

@Axect
Copy link
Owner

Axect commented Jul 30, 2024

One more thing - if you're comfortable with it, I'd like to transfer this issue to the Peroxide repository. This would help us keep track of the implementation process and allow other contributors to follow along or provide input if needed.

Would that be okay with you? If you agree, I'll go ahead and make the transfer.

@JSorngard
Copy link
Contributor Author

JSorngard commented Jul 30, 2024

That sounds like a good idea.

@JSorngard
Copy link
Contributor Author

Completed in #63.

Axect added a commit that referenced this issue Oct 17, 2024
Update `puruspe` dependency, remove `lambert_w` dependency (#64)
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

No branches or pull requests

2 participants