Closed ntjohnson1 closed 1 year ago
Just updated #212. Please update as needed.
Great. I rebased and did a quick check so this should be ready to go now. I elaborated a bit more on the PR description in case that is useful, but feel free to let me know if anything seems weird or I misinterpreted the direction we were discussing for the interfaces.
I notice differences from ktensor
but only in the docs ...
Can you align the docs/source/*.rst
files for the tensor classes with ktensor.rst
. The docs show pyttb.ktensor()
as the class constructor, whereas pyttb.sptensor.sptensor()
is the class constructor with a method called __init__
. The same in the latter case for tensor
and ttensor
. With the changes the documentation makes them appear very different, and I confused these differences for implementation differences. I think the main difference is the exclusion of __init__
, but this should be confirmed.
I notice differences from
ktensor
but only in the docs ...Can you align the
docs/source/*.rst
files for the tensor classes withktensor.rst
. The docs showpyttb.ktensor()
as the class constructor, whereaspyttb.sptensor.sptensor()
is the class constructor with a method called__init__
. The same in the latter case fortensor
andttensor
. With the changes the documentation makes them appear very different, and I confused these differences for implementation differences. I think the main difference is the exclusion of__init__
, but this should be confirmed.
I added two commits for this since there are some stylistic preferences (details are in the commits but adding more info in case it is clearer). For the first I dropped undoc-members
since if we haven't documented things I think we should skip them (this avoids needing to explicitly skip factor_matrices
and other class members). Then I un-excluded __init__
so we actually got the documentation for that.
In the second commit I merged the __init__
docstring with the class doc string since that was more similar to what ktensor looked like before and I think is the desired style.
Since this brings all our tensor types into the same format in the future copy pasting should make new types match as well.
Addresses #187 #188 (and ttensor but I didn't bother with a ticket for that) and #209. Filed #214 as follow up because tenmat's relationships between the constructors is kind of weird, so needs more thought. I followed the design discussed in #145 which I believe was the intention of the first two issues.
There were also three commits https://github.com/sandialabs/pyttb/pull/213/commits/ca9629bf41462b0ebbed56cc059c0e9d8a2cc9d1, https://github.com/sandialabs/pyttb/pull/213/commits/862201b070355b1b7d4dbf8ac6a2465c4e97e795, https://github.com/sandialabs/pyttb/pull/213/commits/2655c829db75450dcff91bc4e0a2a315d295b754 related to simplifying some edge cases in ruff. Basically, we replaced isort with ruff but weren't applying the import sorting/removal of redundant imports to our tests. Also ruff can detect and fix unused variables etc which could be useful if we realize our tests aren't actually doing what we expect.
A few notes for where I set copy=False already. I didn't look at our profiling at all to see if this helped in our performance. I didn't do an exhaustive analysis for where we can skip the copies. Generally I looked for obvious usages where copies were redundant:
~While the content is mostly unrelated I expect this to clash with #212 but they both are based off latest master so feel free to merge whichever you prefer first and I'll resolve the conflicts.~(Resolved)
:books: Documentation preview :books:: https://pyttb--213.org.readthedocs.build/en/213/