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
Head and wings corrections for k-dTDA #58
base: master
Are you sure you want to change the base?
Head and wings corrections for k-dTDA #58
Changes from 19 commits
d05d6b0
83e7741
c4dcf82
2b0bb39
779a964
12929dc
0683748
f7a844e
3431d18
fdd0b30
aff4dc5
ac1ad7e
c7beb4f
26e873b
ad9cdee
43370f5
d0acdee
fe62183
62fd849
cdc2756
32ad7ea
f16b1d9
1fe4a2c
79d8696
f00bbfc
cbe49f8
5d845b4
e5e2b1a
1e6a5bd
7534d99
5b14ad8
e2941f7
d7ebf2d
7f361ee
b2c0fe7
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.
q_abs
is O(1) to compute right? If so, can we make it a property instead.I'd prefer not storing
qij
in the class, but if it's needed in several places then maybe it's for the best. Maybe a more descriptive variable name?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.
Changed the variable name, still not great but not sure what makes more sense.
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.
Same comment as the
moments_dd
. Is it possible to have this function be unchanged, but before we do the convolution, there is something like: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.
Have been able to do this and split
moments_dd
as well.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.
can you explain these changes? does it just intend the output to be contiguous?
my original code allows$(N-1, N-2, ..., 0)$ . This means that in the worst case the new code will require an entire extra copy of
ct
to be in any contiguous order, and then doesn't therein constrain the contiguity ofc
. Sometimes a transpose can just be equivalent to reading the array with the opposite contiguity, if the transpose indices arec
that isn't actually needed.if we want to constrain the contiguity of the output, I think it would be better to just do
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 intention is to try to maintain the contiguous nature of
ct
, particularly given that you can passa
andb
as say c contiguous and then have an output that is not. This is particularly problematic with theao2mo
methods which check the contiguity. Happy though to implement it as mentioned in the comment.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.
This doesn't quite work as the contiguity has already been changed with the transpose, so
order="A"
just keeps it as F contiguousThere 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.
that's fine -- the output doesn't even need to be contiguous really. That being said, I can't think of a situation when it won't be, since it's an output from either C or F implementation of DGEMM.
at
andbt
are either, and the output is fine being either as well.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 can't seem to see it in another PR, but just want to check this isn't change in another PR.
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've not changed it, just revert this on this PR