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

Changed capitalized words to lowercase + some typos #9

Merged
merged 7 commits into from
Dec 20, 2021

Conversation

jvdp1
Copy link

@jvdp1 jvdp1 commented Dec 19, 2021

Based on the reviews, I already changes most of the capitalized words to lowercase words.
I also corrected a few typos, I still discovered.

Note: Capitalized words were not a problem for me... but I guess it is to be uniform across the whole specs.

intrinsic `RANDOM_INIT`.

`odd_random_integer` is intended to generate seeds for
`universal_mult_hash`. `ODD_random_number` uses Fortran's intrinsic
Copy link
Author

Choose a reason for hiding this comment

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

@wclodius2 I couldn't find ODD_RANDOM_NUMBER in the source code. Should it be:

Suggested change
`universal_mult_hash`. `ODD_random_number` uses Fortran's intrinsic
`universal_mult_hash`. `odd_random_integer` uses Fortran's intrinsic

Copy link
Owner

Choose a reason for hiding this comment

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

Its discussion is a leftover from earlier versions of the code. Some no longer included algorithms required an odd random seed. Either you or I can eliminate the discussion of that procedure.

Copy link
Author

Choose a reason for hiding this comment

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

Should it be eliminate from the code too? (probably better I do it before merging, to avoid conflicts)

Copy link
Author

Choose a reason for hiding this comment

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

As I remember, this is needed for universal_mult_hash, which is still provided. So, I think it could remain on the code, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I was wrong. I did a grep on odd_random_number for the source code, and didn't find anything, when, of course, I should have done a grep on odd_random_integer, which finds multiple references in the code. In the documentation the references to odd_random_number should have been changed to odd_random_integer and otherwise retained. I can add them back in if necessary after the merge.

Copy link
Author

Choose a reason for hiding this comment

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

@wclodius2 I changed odd_random_integer to odd_random_number and kept the text there. After resolving a conflict, it should be fine now.

Copy link
Owner

Choose a reason for hiding this comment

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

Odd, I had suggested changing odd_random_nuber to odd_random_integer, while you claim to have done the reverse. FWIW I have no problems with that if you also changed it in the source code.

Copy link
Author

Choose a reason for hiding this comment

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

I am sorry. I meant "I changed odd_random_number to `odd_random_integer", as you proposed.
Again sorry for the confusion.

Copy link
Owner

Choose a reason for hiding this comment

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

Great!

@wclodius2
Copy link
Owner

Do you think you are ready to be merged?

@jvdp1
Copy link
Author

jvdp1 commented Dec 20, 2021

Do you think you are ready to be merged?

Yes, it can be merged.

@wclodius2 wclodius2 merged commit 95fcc8f into wclodius2:hash_functions2 Dec 20, 2021
@jvdp1 jvdp1 deleted the myhash14 branch December 22, 2021 16:54
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