Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Last minute cleanups #1244
Last minute cleanups #1244
Changes from 9 commits
9b5e18b
e4a225b
f1931c0
cf59c30
9df17f5
5398281
393c583
fdd1fd5
b7e8733
ab2001e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these should be typed as
Slot
orEpoch
. "5 slots" is not actually aSlot
. It's a count of something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the thing that is being counted here not a
Slot
though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slot
was calledSlotNumber
, as the ID to identify a certain slot;Epoch
andShard
are as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I disagree:
EPOCHS_PER_...
is consistent with theEpoch
type.EPOCHS_PER_HISTORICAL_VECTOR
naturally occurs inEpoch
summations such asepoch + EPOCHS_PER_HISTORICAL_VECTOR - MIN_SEED_LOOKAHEAD
.EPOCHS_PER_HISTORICAL_VECTOR
naturally occurs inEpoch
modular reductions such asepoch % EPOCHS_PER_HISTORICAL_VECTOR
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in instances where there is a
PER
in the name, dimensional analysis dictates that they are notEpochs
, but `Epochs * HistoricalVectors**(-1).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SLOTS_PER_EPOCH
is defined as "the number of slots in an epoch". That's of typeSlot
, notSlot/Epoch
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree. The use of
SLOTS_PER_EPOCH
inslot_to_epoch
and inepoch_start_slot
stand counter to your argument.If
SLOTS_PER_EPOCH
is aSlot
thenepoch_start_slot
's return type isEpoch*Slot
andepoch_start_slot
is dimensionless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historical vector here doesn't represent a type, but a non-typed scalar. So you could say we are multiplying by something that doesn't change the type, only the length. Imho I think the debate is silly, and naming is consistent and clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, OK. Would you agree that
Epochs * HistoricalVectors**(-1)
is the same asEpochs
becauseHistoricalVectors
is dimensionless? 😂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't care too much either way. Need to go to sleep soon as I can't think straight anymore 💤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂Fine. :) I am not entirely convinced, but I sort of agree enough on the dimensionlessness
HistoricalVectors
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed the need for sleep is real.