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
Add Covering Edges #15
base: master
Are you sure you want to change the base?
Add Covering Edges #15
Changes from all commits
80062c8
ba43c25
c438316
139c3fc
3d8eca6
43351b9
078790a
6aba883
7209db3
ecb99ad
bb037e9
f72bcad
166c677
20fa77f
8907323
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.
use of private API
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.
Any tips how to avoid that? Problem is that this whole function is probably also for internal use only. Maybe the whole example can be defined other way, originally wanted to avoid using
fast_generate_from
in the example, but what do you think? That would remove this line. Example could be context ->fast_generate_from
->ConceptList
->covering_edges
.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 am confused: Can't the function just use the output of a concept generator like
fcbo
as input?See the other discussion on avoiding materialization. :)
Although, I might be missing something and need to read the paper first.
In case a test fiddles with internals or otherwise has a complex setup, state requirement, or assertions, it should probably be done in
tests/
. Doctests are better for simple (pure) functions and if they are used as an example, they should only use official API (in general, I think a good API is one that just flows naturally in the repl). As this is a pure function, my hope is we can have the doctest both guard the implementation and serve to understand the API.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.
Another option might be to create the input from scratch in the test (creating the two
bitsets.bitset
s).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.
The reasons why context is needed are the prime operators and bitset classes.
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.
Ah, right, that is a clever solution, thanks. I will add that and remove context.
PS: So do we want to force
covering_edges
to accept an iterator? Inside it will be transformed into dict anyway, but that would allow directly passing fcbo output.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 have checked the comments once again. So, do you agree, that
covering_edges
would yieldNamedTuple
's:I am still confused what is the purpose of
concepts._common.Concept
but it would be useful here.Other option is (hope I got typing for bitset right):
As for input for
covering_edges
, I would be for:In that case,
fast_generate_from
can be directly plugged in. Inside function, your example (#15 (comment)) would be used. Socovering_edges
assumes, that pairs of extent/intent will be plugged in, not concepts related to some lattice.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.
The idea mentioned in #15 (comment) is interesting, but I am worried it would be better to build full lattice from
ConceptMappingInfo
in next step.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.
Quick notes:
Still nesting (yield quadruples, see earlier discussion)?
IIUC this differs from
Yield mapping edge as ``((extent, intent), (lower_extent, lower_intent))
, is it intended to include both the extent and the intent of neighbors even though one of both suffices for a reference?Same here:
nit:
typing.Iterable
(prefer generic types for arguments e.g.Sequence
and concrete ones for returns, e.g.List
, cf. Postel's law)We might need to do some more to express the requirement that the bitset pairs need to be ones that where for prime in
matrices.py
.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.
You right, that both extent/intent are not required for storing the relationship, so probably something like:
Agree with
typing.Iterable
as input. Maybe we will get better big picture when we will try to build whole lattice with this function? Then we can modify the API more and keep it public or make it fully internal only function.Do you agree that adding
lattice
function intocovering_edges.py
in the same fashion as inlindig.py
would be good approach? That function would acceptcontext
as argument, because internally usesfast_generate_from
to generate concepts andcovering_edges
to build mapping (that is main difference to the one in thelindig.py
output will be same). Thislattice
function should be easily pluggable into existing data structure.Then we can finish the whole code.