Skip to content
This repository was archived by the owner on Sep 20, 2021. It is now read-only.

Added constants to Arithmetic - #20

Closed
jremes-foss wants to merge 2 commits into
hoaproject:masterfrom
jremes-foss:master
Closed

Added constants to Arithmetic#20
jremes-foss wants to merge 2 commits into
hoaproject:masterfrom
jremes-foss:master

Conversation

@jremes-foss

Copy link
Copy Markdown
Contributor

I added some constants to Arithmetic.php. Thank you and have a nice weekend. :-)

Comment threadVisitor/Arithmetic.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we fine any better names for 1_PI, 2_SQRTPI and 1_SQRT2?

@HywanHywan self-assigned this Jan 9, 2015
@Hywan

Hywan commented Jan 9, 2015

Copy link
Copy Markdown
Member

Nice PR, thanks 👍!

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Hi. You're welcome. I might actually change the names, but there is a logic - LaTeX variable naming convention. 1_PI = 1 / PI, etc.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 Yup, but the actual grammar does not allow to start a constant name by an integer. Do you think we can use another logic here?

We face the same issue here: hoaproject/Ruler#32.

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Yes, we certainly can. Thanks for notifying me about this, I think I can checkout the latest repo and re-write the array.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why INVERSE?

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 Thank you for your effort :-)! An alternative would be to allow constant to start by an digit if renaming constants is too hard.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 ping 😄?

@Hywan

Copy link
Copy Markdown
Member

So constants will not start by a digit. See hoaproject/Ruler#32 and associated PR.

I would like to merge this PR. If there is no pong, I will merge and do the fix myself :-).

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry if I have been inactive - super busy with work. Pong, I will do this tonight or tomorrow morning.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 No stress, it's open-source :-).

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 ping ❤️?

1 similar comment
@Hywan

Copy link
Copy Markdown
Member

@NewMan101 ping ❤️?

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Ok, let me just re-write this one to get the naming convention 100% accurate. Sorry for delay.

@Hywan

Copy link
Copy Markdown
Member

No worry :-). Just would like to know if we finish this PR for you or not. Feel free!

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

I think I will create a fresh PR from a scratch and new branch after reviewing the naming conventions.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 Why? This is still mergeable. Just run git rebase master if you want an up-to-date codebase.

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Ok, well... If this is still mergeable I'll do it then using this PR. As far as I know the only issue is that the grammar does not allow to start a constant name by an integer.

@Hywan

Copy link
Copy Markdown
Member

Yes, the naming might need to be clarify. I will add new comments bellow.

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Ok, cheers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we follow the existing “logic”, we should have LOG_2_E instead of LOG2E, or maybe LOG_2E.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I chose LOG_2E

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Huh, that's a really strange value :-/. When is it useful? If not useful, please remove it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kept this constant but renamed it TWO_DIV_SQRT_PIINVERSE_SQRT_PI

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Yea, that's done now... I just need to rebase the whole branch now. I got rid of the last value.

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Only issue is that the original branch was deleted from my side. So the branch is "unknown branch" at the moment.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 Oh. You can still get the patch and apply it on a new branch. Tips: https://patch-diff.githubusercontent.com/raw/hoaproject/Math/pull/20.patch.

@Hywan

Copy link
Copy Markdown
Member

@NewMan101 Hi :-). Is it possible to address the comments please? If not, we will do that by ourselves :-).

@jubianchi

Copy link
Copy Markdown
Member

@Hywan I can do this if you want ;)

@Hywan

Hywan commented Mar 4, 2016

Copy link
Copy Markdown
Member

@jubianchi Please do!

@jremes-foss

Copy link
Copy Markdown
ContributorAuthor

Feel free to do that. I think I've lost the relevant data.

This was referenced Mar 7, 2016
@BhoatBhoat closed this in #38 Jan 20, 2017
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

3 participants

@jremes-foss@Hywan@jubianchi