Skip to content

Add script for generating test data, and a test of lambert_w0, sp_lambert_w0, lambert_wm1, sp_lambert_wm1#15

Merged
Axect merged 6 commits intoAxect:v0.3.0from
JSorngard:v0.3.0
Aug 6, 2024
Merged

Add script for generating test data, and a test of lambert_w0, sp_lambert_w0, lambert_wm1, sp_lambert_wm1#15
Axect merged 6 commits intoAxect:v0.3.0from
JSorngard:v0.3.0

Conversation

@JSorngard
Copy link
Copy Markdown
Contributor

@JSorngard JSorngard commented Aug 5, 2024

This is my proposal for a test of the Lambert W functions.

@Axect
Copy link
Copy Markdown
Owner

Axect commented Aug 5, 2024

The code appears to be perfect. However, as it's currently midnight in Korea, I've already left the lab for the day. I'll thoroughly review and incorporate your changes after I return to work tomorrow morning. I kindly ask for your understanding in this matter.

@JSorngard
Copy link
Copy Markdown
Contributor Author

Of course. I will work on moving the functions and their tests into their own file in the meantime.

@JSorngard JSorngard changed the title Add script for generating test data for W_0, and add a test of lambert_w and sp_lambert_w Add script for generating test data and a test of lambert_w0, sp_lambert_w0, lambert_wm1, sp_lambert_wm1 Aug 6, 2024
@JSorngard JSorngard changed the title Add script for generating test data and a test of lambert_w0, sp_lambert_w0, lambert_wm1, sp_lambert_wm1 Add script for generating test data, and a test of lambert_w0, sp_lambert_w0, lambert_wm1, sp_lambert_wm1 Aug 6, 2024
@Axect
Copy link
Copy Markdown
Owner

Axect commented Aug 6, 2024

Thank you for your excellent contribution, @JSorngard! Your work on implementing tests for the Lambert W functions is truly impressive. I'd like to share some thoughts on your changes and propose a few adjustments. I'd greatly appreciate your input on these ideas:

  1. Test Implementation: Your tests are exemplary. They thoroughly validate the accuracy of the Lambert W functions, which is crucial for our library. Great job on this!

  2. Module Structure: While I appreciate the effort to organize the code, I'm concerned that the current structure with local_lambert_w.rs might become complex if we add more external dependencies. Instead, I propose simplifying lib.rs to just:

    pub use lambert_w;

    This approach is clean and allows for intuitive usage:

    use puruspe::gamma::ln_gamma;
    use puruspe::lambert_w::lambert_w0;

    The crate name 'lambert_w' is already intuitive, so we don't need to use an alias.

  3. Test Location: I suggest moving the tests to a separate file, tests/lambert_w_test.rs. This would keep our test code organized while maintaining a simple main library structure.

  4. Prelude: We could create a prelude for users who want to import all functions at once. What do you think about this idea?

If you're comfortable with these suggestions, I'd like to proceed with the following changes:

  1. Simplify the module structure by removing local_lambert_w.rs and updating lib.rs as shown above.
  2. Move the tests to a new tests/lambert_w_test.rs file.
  3. Create a prelude for convenient imports.

I value your input and would be happy to discuss any concerns or alternative ideas you might have. If these changes sound good to you, I plan to first merge your current pull request and then start working on these additional modifications directly in the repository.

Once again, thank you for your valuable contribution and the impressive accuracy of the Lambert W functions. Your work significantly enhances the quality of our library.

@JSorngard
Copy link
Copy Markdown
Contributor Author

JSorngard commented Aug 6, 2024

I think the file restructure and module renaming is a very good idea. It would make it clearer, and simpler.

Moving the tests make sense I think.

Regarding the prelude, would it not work for the user to do use puruspe::*; to import everything? I interpreted the different function specific modules to not be exposed to the user, but just be internal organization so that files don't get too big. From a user perspective everything would be organized like now. Though, I could of course have misinterpreted things.

@Axect
Copy link
Copy Markdown
Owner

Axect commented Aug 6, 2024

Thank you for your feedback. I really appreciate your insights.

  • I'm glad you agree with the file restructure and module renaming ideas. Your support for these changes is valuable. I'll proceed with merging the current PR and then immediately start working on these modifications.

  • You're absolutely right about the current functionality of use puruspe::*;. It's already working as intended and is indeed a reasonable approach. After considering your point, I agree that adding a separate prelude would be unnecessary.

  • Your suggestion to keep the direct exports in lib.rs to allow users to simply use use puruspe::*; is spot on. It maintains simplicity while providing easy access to all functions. We'll stick with this approach rather than introducing a new prelude.

Thank you again for your contributions and thoughtful feedback. It's helping us make puruspe more intuitive and user-friendly. I'll start implementing these changes right after merging the current PR.

@Axect Axect merged commit b5963f0 into Axect:v0.3.0 Aug 6, 2024
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