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

rename and doc finSubCover #832

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Conversation

affeldt-aist
Copy link
Member

Motivation for this change

fixes #658

I added a documentation entry.
Since the naming was questioned, I propose a new one that essentially gets rid of camlCase
which is more commonly used for mixins and structures in MathComp.
I haven't changed the definition as suggested by the issue.

Things done/to do
  • added corresponding entries in CHANGELOG_UNRELEASED.md
  • added corresponding documentation in the headers
Automatic note to reviewers

Read this Checklist and put a milestone if possible.

@affeldt-aist affeldt-aist requested a review from t6s January 31, 2023 14:02
@affeldt-aist
Copy link
Member Author

Quote from issue #658:

The name suggests that it is pinpointing a (finite) subcover of some cover, but there is no such original cover in the definition.

Indeed.
At first I thought of renaming to finite_subset_cover so that it sounds more like we take the subset of some set but this might look like it points to the definition of cover in classical_sets.v that we do not use here.
Hence this minima change.

@affeldt-aist
Copy link
Member Author

Or maybe we can try to use cover: 62440d9

And then the finite_subset_cover makes more sense.

@affeldt-aist
Copy link
Member Author

@zstone1 do you think that this is a reasonable change?

Copy link
Contributor

@zstone1 zstone1 left a comment

Choose a reason for hiding this comment

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

Looks good to me. That name always threw me off.

- use cover in finSubCover
- use [set: ] instead of @Sett
- use shortcut notation for set comprehension
@affeldt-aist
Copy link
Member Author

@t6s do you think this PR fixes your issue?

@affeldt-aist affeldt-aist merged commit 22c300b into math-comp:master Feb 7, 2023
@affeldt-aist affeldt-aist deleted the fixes_658 branch February 7, 2023 02:41
hoheinzollern pushed a commit to hoheinzollern/analysis that referenced this pull request Mar 9, 2023
- use cover in finSubCover
- use [set: ] instead of @Sett
- use shortcut notation for set comprehension
proux01 pushed a commit that referenced this pull request Mar 24, 2023
- use cover in finSubCover
- use [set: ] instead of @Sett
- use shortcut notation for set comprehension
proux01 pushed a commit that referenced this pull request Mar 24, 2023
- use cover in finSubCover
- use [set: ] instead of @Sett
- use shortcut notation for set comprehension
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.

finSubCover lacks documentation
3 participants