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

141 composite vector space #144

Merged
merged 11 commits into from
Feb 21, 2024
Merged

141 composite vector space #144

merged 11 commits into from
Feb 21, 2024

Conversation

eparish1
Copy link
Contributor

@eparish1 eparish1 commented Feb 15, 2024

addressing #141

@fnrizzi
Copy link
Member

fnrizzi commented Feb 16, 2024

Why is a composite vec space not inheriting from vector space?
I think it should

@eparish1
Copy link
Contributor Author

@fnrizzi I broke up the constructor and made this inherit from VectorSpace. I think these both make sense, although the inheritance will probably go away if we switch to protocols.

@fnrizzi
Copy link
Member

fnrizzi commented Feb 19, 2024

IMO we should not allow for a "compact_representation" to be true for these reasons:

  • all this does is to get the basis from each individual vec space which one can do outside as easily. Havign this class does not add a layer of abstraction that makes it worth it IMO.

  • if we provide a get_compact_basis we kind of break the vector space concept. So this breaks our usage internally because we cannot just rely on the interface of vector space get_basis.

I would suggest to make this strictly a global composite vector space.

@eparish1
Copy link
Contributor Author

@fnrizzi We can make this a global composite vector space, but then need to have another class that is compact for a variety of reasons. I'm fine with either. Do you think the latter is better? The classes will basically be the same, but the compact one won't have a get_basis.

@fnrizzi
Copy link
Member

fnrizzi commented Feb 19, 2024

@fnrizzi We can make this a global composite vector space, but then need to have another class that is compact for a variety of reasons. I'm fine with either. Do you think the latter is better? The classes will basically be the same, but the compact one won't have a get_basis.

can you explain to me later why we need a compact one? the compact one without a get_basis is not usable for any API that requires a vector space

@fnrizzi
Copy link
Member

fnrizzi commented Feb 19, 2024

the compact one is just a list of basis that is created by calling get_basis on each

@eparish1
Copy link
Contributor Author

@fnrizzi If we don't have a compact one, then we need to have completely different notions for what inputs to the algorithms are. Regardless of how we represent the basis, the collection of vector spaces forms the same concept.

What are your thoughts if we move the construction of the dense basis to the "get_basis", and only do it once the first time it is called. This way there is no notion of True or False for compact representation and we meet the vector space API. We can then still offer the get_compact_basis as an extra function.

@fnrizzi
Copy link
Member

fnrizzi commented Feb 19, 2024

Can we talk later about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should think/talk about this variable ordering thing. The normal VectorSpace concept doesn't include anything about variable ordering and I'm not 100% sure we need/want to support it here (vs just assuming that they pass in the VectorSpaces in a consistent ordering.

@eparish1
Copy link
Contributor Author

@jtencer @fnrizzi

I updated John's compromise with some of the stuff we talked about. Can you please see if you agree? Only thing left on my mind: should we add logic so that we only construct the dense basis once if we call get_basis()? (e.g., we construct it and store it as a member in the class and just return it if get_basis gets called multiple times?). What do you think?

@cwschilly cwschilly mentioned this pull request Feb 20, 2024
@eparish1 eparish1 merged commit 75b83b9 into develop Feb 21, 2024
6 checks passed
@fnrizzi fnrizzi deleted the 141-composite-vector-space branch February 21, 2024 07:54
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.

3 participants