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
[Merged by Bors] - feat(ring_theory/discrete_valuation_ring): add additive valuation #5094
[Merged by Bors] - feat(ring_theory/discrete_valuation_ring): add additive valuation #5094
Changes from 6 commits
d08a2ab
c4feee7
e737e5c
99017e0
19e12f2
9a292e7
0f2ab70
65cfe11
c0f4026
c0dd4aa
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.
There's not much point, but you could declare this a
zero_hom R ℕ
to get a tiny bit of bundlingThere 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 almost sent 0 to 37. This definition is really an auxiliary one; I will only be using it when defining the correct object, which is either the map to
with_top nat
sending 0 to top, or (more likely) the map towith_zero (multiplicative nat)
orwith_zero (multiplicative int)
, because the latter is thevaluation
right now (and the former isn't, but will be once I get round to generalising the definition ofvaluation
).Should I rename this function
add_val_aux
and define a new functionadd_val
towith_top nat
? This would have more functorial properties.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 adding
_aux
to the name is a useful flag that this is meant as implementation detail. You could also explicitly state that in the docstring.Otoh, if we think we'll be using this later on, then I'd prefer to keep the name the way it is.
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.
add_val_def
is the more friendly version intended for general use, right?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.
Yes,
add_val_def
is the useful version. Even though it's not the actual definition, it's the "mathematician's definition" of the additive valuation. I'm not sure ifadd_val_spec
is a specific instantiation ofadd_val_def
-- I'm not really sure I understand what that means.