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

Adding more math constants (and a small correction to the description of an existing one) #9181

Merged
merged 7 commits into from
Jun 1, 2023

Conversation

simdimdim
Copy link
Contributor

@simdimdim simdimdim commented May 12, 2023

Adding more float constants for when rust-lang/rust#103883 is accepted and merged. And fixing a small conflation in the description of the Euler number. Please take a look and let me know if I've missed or screwed up anything.

@simdimdim simdimdim marked this pull request as ready for review May 12, 2023 14:53
Copy link
Contributor Author

@simdimdim simdimdim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo fix, missed an 'e'

@amtoine
Copy link
Member

amtoine commented May 12, 2023

@simdimdim
looks like you should run

cargo fmt --all

or

use toolkit.nu
toolkit fmt

😉

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the CI will be happy, this looks good to me 👌

we have pi and e why not gamma and phi 😋

Note
i'm just wondering if we should keep adding constants to math in the long run 🤔
maybe nu_scripts or the standard library might be a better fit later 👍

@simdimdim
Copy link
Contributor Author

Well, ultimately we still have to wait for rust-lang/rust#103883 and I have no clue how to make the pull request conditionally depend on it.

for the record

use toolkit.nu
toolkit fmt

where can I do that?

@amtoine
Copy link
Member

amtoine commented May 12, 2023

use toolkit.nu
toolkit fmt

where can I do that?

from inside a Nushell instance, from the root of the nushell/nushell repo on your local machine 👌

@WindSoilder
Copy link
Collaborator

Hi @simdimdim thanks for your contribution. Since the pr is adding some math consts, I'd suggest to make it to be nu-std's module

@simdimdim
Copy link
Contributor Author

Hi @simdimdim thanks for your contribution. Since the pr is adding some math consts, I'd suggest to make it to be nu-std's module

Hey o/,
I just added them where the other math constants were. If they get moved, I'll submit a new commit with the appropriate changes, since I'm very unfamiliar with the codebase and pretty much just copied pi.rs and euler.rs and edit their contents.

@fdncred
Copy link
Collaborator

fdncred commented May 18, 2023

I'm fine with landing this if the ci gets green. We can figure out what to move to stdlib after this.

@simdimdim
Copy link
Contributor Author

well, that's it on my end I think. Now we just wait for the rust std to add PHI and EGAMMA to std::f64::consts::

@fdncred
Copy link
Collaborator

fdncred commented May 18, 2023

well, that's it on my end I think. Now we just wait for the rust std to add PHI and EGAMMA to std::f64::consts::

I don't get this. If PHI and EGAMMA aren't in std::f64::consts, why do we have this PR? We won't be able to land this if they don't exist because the CI won't be green and the code won't work, right?

@WindSoilder
Copy link
Collaborator

well, that's it on my end I think. Now we just wait for the rust std to add PHI and EGAMMA to std::f64::consts::

I've checked these consts in both stable and nightly doc, but it don't exists in both place, and I don't think they'll be added in forseen future. Are there any rfc or pr in rustc to add these constants?

@simdimdim
Copy link
Contributor Author

simdimdim commented May 18, 2023

well, that's it on my end I think. Now we just wait for the rust std to add PHI and EGAMMA to std::f64::consts::

I've checked these consts in both stable and nightly doc, but it don't exists in both place, and I don't think they'll be added in forseen future. Are there any rfc or pr in rustc to add these constants?

@WindSoilder, @fdncred ,
I've mentioned the related issue on the rust side in the first message already: rust-lang/rust#103883
(and mentioned I dunno how to make this issue/pr depend on the status of the other one so it can automatically become mergeable when they're ready with the changes on the rust side of things). Regards.

@WindSoilder
Copy link
Collaborator

Oh, sorry for missing that message...Then I'll mark it as a draft.

@fdncred
Copy link
Collaborator

fdncred commented May 19, 2023

Just add these and be done with it, right?

/// The golden ratio (φ)
pub const PHI: f64 = 1.618033988749894848204586834365638118_f64;

/// The Euler-Mascheroni constant (γ)
pub const EGAMMA: f64 = 0.577215664901532860606512090082402431_f64;

@fdncred fdncred marked this pull request as draft May 19, 2023 02:05
@simdimdim
Copy link
Contributor Author

Just add these and be done with it, right?

/// The golden ratio (φ)
pub const PHI: f64 = 1.618033988749894848204586834365638118_f64;

/// The Euler-Mascheroni constant (γ)
pub const EGAMMA: f64 = 0.577215664901532860606512090082402431_f64;

That works too, should I do that instead?

@fdncred
Copy link
Collaborator

fdncred commented May 19, 2023

if you want to land this PR, i think it's the only way to move forward right now.

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2023

@simdimdim Do you have time to update this PR with what is discussed above?

@simdimdim
Copy link
Contributor Author

clippy appears to be unhappy with the too many decimal places, should I also do as it suggests?

@fdncred
Copy link
Collaborator

fdncred commented May 31, 2023

I'm not sure of the ramifications but my first reaction is to do what you did and add an allow.

@simdimdim
Copy link
Contributor Author

well, since it's meant to be only a temporary patch it shouldn't really matter (once the constants could be used directly from std)
I think the actual values would get truncated anyway (to something that fits in a f64).
But for the time being I think this is all.

@simdimdim simdimdim marked this pull request as ready for review May 31, 2023 22:51
@fdncred
Copy link
Collaborator

fdncred commented Jun 1, 2023

Thanks. It's fine for now. It's likely that some of these basic type math commands will be moved to our std lib anyway.

@fdncred fdncred merged commit 2f731fa into nushell:main Jun 1, 2023
@simdimdim simdimdim deleted the more_float_constants branch June 1, 2023 01:44
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.

4 participants