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

Additional IndexSet features (union, intersection, difference, symmetric difference, select, filter, vector) #210

Open
wants to merge 3 commits into
base: v2
Choose a base branch
from

Conversation

1ucasvb
Copy link

@1ucasvb 1ucasvb commented Mar 12, 2019

I've been working on some high-rank tensors with many indices of various types, and I find that ITensor was lacking a way to easily manipulate many indices at once. The functions commonIndex and findtype only return the first match, which is completely insufficient in such cases where multiple indices are required.

These additions to the IndexSet class allow us to perform these set-based operations quickly and easily, and immediately use them on any function which accepts IndexSet as an input. If they don't (unfortunately, some do not), there's also a .vector() method which returns the vector of indices which is typically accepted.

Example

Given two IndexSetinstances A = {i,j,k,l} and B = {k,l,m}

  • A+B = {i,j,k,l,m}: collect all unique indices from two sets
  • A*B = {k,l}: indices in both sets (this extends commonIndex)
  • A-B = {i,j} and B-A = {m}: indices only on A or only on B

The .select() and .filter() methods allow the user to further manipulate such sets by type.

See code comments for more details. Proper unit tests were also included in these commits.

@emstoudenmire
Copy link
Contributor

Hi Lucas, thanks for this. I agree it's definitely needed - we just usually have an approach of not adding things until we know of a particular use-case for them. Since you have identified one for your own code, we should add this.

I'd like to make some changes, though, including the following:

  • replace the operators +, -,* with functions named union, intersection, difference (not sure about the last name; happy for you to suggest a better one). One reason for this is then the code will more more self-explanatory to readers.
  • also as you noted, the implementations of some of the functions could be improved. For example, in C++ there is a std::array type which is much preferable to C-style arrays. Also one can probably get away with not using unordered_map, but that's just an optimization - not a big deal.

Matt or I can make those changes though. However, you are free to take a swing at them too if you are so inclined! I added your name to the website list of contributors for the other pull & will give you a badge for this one too. Thanks.

@1ucasvb
Copy link
Author

1ucasvb commented Mar 12, 2019

EDIT: I went ahead and decided to give it a go. I think everything should be OK now!

Hello, Miles. I totally understand the approach, it's very reasonable. I'm glad you also find this useful.

As for the changes you suggested, they are all fine with me. I used +\-\* for simplicity in my code, but I understand your take on it. I think union, intersection and difference is just fine, and in fact these are the same names Python uses for its set operations.

(Edit: since union is a reserved keyword, I opted for a more explicit naming convention.)

There's also symmetric_difference, which I didn't have to use, so didn't implement, but it's easily achievable with the others anyway. This one would be equivalent to (A union B) difference (A intersection B) (but that can be done faster directly), so it could be useful for predicting the IndexSet of the contraction of two tensors, as the resulting IndexSet removes all the common indices. I figure we should probably add that too.

(Edit: this is now implemented in the latest commit.)

Regarding the use of C arrays\unordered_map, I knew my implementation wasn't going to be very good or modern, but since I'm still learning these details on modern C++ programming I couldn't offer anything better at the time. Please, feel free to re-implement these as you think works best. it would in fact be very valuable to see how more experienced programmers would do it!

(Edit: I actually decided to give it a go, and I think it should be fine now.)

I added your name to the website list of contributors for the other pull & will give you a badge for this one too.

Thank you! If you could change it to "Lucas Vieira" at Federal University of Minas Gerais, following the convention of others in the list, and link to my site instead, I'd appreciate it.

(Edit: I also took the liberty of making a pull request for these changes.)

@1ucasvb
Copy link
Author

1ucasvb commented Mar 13, 2019

OK, I decided to give it a go and rewrite the functions to have better names and better implementations. Since union is a reserved word and there's really no other way of calling it, I decided to use .setUnion() and similar for the rest.

I also added symmetric difference for completeness (also in the unit test), and created another pull request for the documentation of all these features as per this last commit.

I think they should all be OK now, and they're only using internal features of the library. It's very self-contained! :)

I hope this is good enough work. Cheers!

@1ucasvb 1ucasvb changed the title Additional IndexSet operations (union, intersection, difference, select, filter, vector) Additional IndexSet features (union, intersection, difference, symmetric difference, select, filter, vector) Mar 13, 2019
@mtfishman
Copy link
Member

Hi, this pull request is a very good idea, and I think it can help clean up a lot of code! I want to point out that I recently added very similar functionality to ITensor V3 (on the rc3 branch). The functions I added can be found here: https://github.com/ITensor/ITensor/blob/rc3/itensor/indexset.h#L709, and the implementations are in itensor/indexset.cc.

I think it covers all of the functions you added in this pull request. I chose some non-traditional names for these operations, to be consistent with previous definitions of similar functions that have been around in ITensor (for example, I use the name commonInds for the set intersection, since commonIndex has been around in ITensor a while).

Also, I prefer these to be free functions rather than member functions, since we are trying to stick to a convention that member functions are ones that modify while all other functions are free (and these all return new IndexSets, rather than modify an input IndexSet). Would you mind updating this pull request to reflect the same names and styles as the V3 versions, so that we can have this functionality in V2 as well?

Also note that in V3, I have added conversion of various collections like std::vector<Index>, std::array<Index,N>, std::initializer_list<Index> to IndexSet, so now functions that are defined in terms of accepting IndexSet will also accept those collections too. I would encourage you to try out ITensor V3 since it has a lot of features that add more user control of Indices. We are still in the process of documenting it, but it should be finished in the next week or so.

Cheers,
Matt

@1ucasvb
Copy link
Author

1ucasvb commented Mar 23, 2019

Excellent, Matt! I'll make the changes you requested and update the request.

I haven't looked much into V3 yet, but I think I'll definitely keep an eye on it now.

Cheers!

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.

None yet

3 participants