Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

feat(order/filter/filter_product): build hyperreals#801

Merged
avigad merged 6 commits intomasterfrom
filter_product
Mar 17, 2019
Merged

feat(order/filter/filter_product): build hyperreals#801
avigad merged 6 commits intomasterfrom
filter_product

Conversation

@abhimanyupallavisudhir
Copy link
Copy Markdown
Collaborator

Construction of "filter products" (ultraproducts on a general filter), ultraproducts, some instances, hyperreal numbers. Not sure if it belongs in another folder.

@sgouezel
Copy link
Copy Markdown
Collaborator

sgouezel commented Mar 8, 2019

Comments on the form:

  • You should add a header, with the license details and so on, but also explaining what is in the file, with motivations, math explanations, and references to the literature.
  • Add docstrings to most of the definitions
  • You should use namespaces: put the first definitions in the filter namespace, and the instances maybe in filter.bigly_equal.

Otherwise, the maths look good (except that you are missing a field instance when your filter is a ultrafilter).

Copy link
Copy Markdown
Collaborator

@johoelzl johoelzl left a comment

Choose a reason for hiding this comment

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

Hi @abhimanyupallavisudhir,
the formalization looks really nice. I have a couple of style comments and requests for changes.

Construction of filter products, ultraproducts, some instances, hyperreal numbers.
@avigad
Copy link
Copy Markdown
Collaborator

avigad commented Mar 16, 2019

@abhimanyupallavisudhir I already made the fix and checked compilation on my machine. I am just waiting for the Travis check to finish, and then I'll merge.

@abhimanyupallavisudhir
Copy link
Copy Markdown
Collaborator Author

@abhimanyupallavisudhir I already made the fix and checked compilation on my machine. I am just waiting for the Travis check to finish, and then I'll merge.

Isn't making f implicit a better fix?

Copy link
Copy Markdown
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

I realise I'm a bit late with these comments, as @avigad was already talking about merging this. I don't mind if you ignore these comments... they're not that important.

I should stress that I think this is a cool PR!

@avigad
Copy link
Copy Markdown
Collaborator

avigad commented Mar 16, 2019

@abhimanyupallavisudhir I didn't even see your change. I think we pushed them at the same time. Could you make the argument implicit again and adapt @jcommelin's suggestions as you see fit? Then I'll merge right away.

@abhimanyupallavisudhir
Copy link
Copy Markdown
Collaborator Author

Ok, all done.

@avigad avigad merged commit e8bdc7f into master Mar 17, 2019
@johoelzl johoelzl deleted the filter_product branch March 19, 2019 10:11
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 15, 2020
…nity#801)

Construction of filter products, ultraproducts, some instances, hyperreal numbers.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants