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

[WIP] DerivableInterfaces refactor #39

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

lkdvos
Copy link
Contributor

@lkdvos lkdvos commented Feb 19, 2025

This is a WIP implementation of some of the things that were discussed, also in light of the upcoming DerivableInterfaces changes.

Builds on top of #31.

For now, it mostly contains a more built out version of the indexing, with a little more care towards boundschecking.
I have not yet started testing, just wanted to get an initial implementation going to allow for possible early feedback.

Notable changes:

  • isstored, set/get(un)storedindex now work for cartesian, boolean and linear indices, since they have more support via to_indices and a canonical entry point
  • As the AbstractSparseArrayInterface file became rather unwieldy, I split off the parts that are just generic indexing functions into a separate file
  • Docstrings
  • boundschecks
  • no more dependency on the @derive T trait machinery

@mtfishman
Copy link
Member

Looks like a great start! Definitely more transparent this way.

@lkdvos lkdvos force-pushed the derivableinterfaces branch from 5a8301c to af37448 Compare February 26, 2025 22:17
@lkdvos lkdvos force-pushed the derivableinterfaces branch from af37448 to caee0b1 Compare February 26, 2025 22:18
@lkdvos
Copy link
Contributor Author

lkdvos commented Feb 26, 2025

I have to apologize for the mess: this PR is somewhat growing uncontrollably.
I'm developing this in tandem to ITensor/DerivableInterfaces.jl#28 as a proof that there is no missing functionality there.
I started making some of the required changes to do so, but somehow I might have ended up bundling too many things together...

It would be great if you could maybe give your opinion on where this is going.
Notable new things here: I added an initial attempt for the "zero-preserving" functions etc, based on the discussion we had last week and the implementation that was already here.
Again, since this got rather larger, I opted to separate that out into a file.
(also, it looks like I broke broadcasting again 🎉 )
Anyways, definitely needs more work, but I think it would be good to rediscuss before this grows completely out of control.

@mtfishman
Copy link
Member

I presumed that this would be large PR given how disruptive ITensor/DerivableInterfaces.jl#28 is. It all looks pretty reasonable to me so far, maybe it will just warrant you giving me a summary once the dust settles a bit with the two PRs. But right now the scope seems fine, unless you want to split it up into separate PRs for your own sanity.

I see basically there are:

  1. updates reflecting [WIP] v0.3 refactor DerivableInterfaces.jl#28, for example making derive calls more explicit,
  2. changes to constructors, and
  3. changes to how zero preservation is handled in map/broadcast.

So maybe 2. and 3. could be split off into their own PRs since they look pretty self-contained.

@mtfishman
Copy link
Member

(I forgot that the constructors are already in a separate PR, i.e. #33, so that will already simplify this PR when that gets merged.)

@mtfishman mtfishman mentioned this pull request Feb 27, 2025
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.

2 participants