Closed zerothi closed 5 months ago
Looks good to me
Thanks!
One final thing that occurred to me. Generally we have used $\mathbf R$ as lattice vectors, and $\mathbf r$ as atomic coordinates. So now, what should we do? $\mathbf R\alpha$ to signal lattice matrix, and an index, and then $\mathbf r\alpha$ to signal coordinates?
One final thing that occurred to me. Generally we have used R as lattice vectors, and r as atomic coordinates. So now, what should we do? Rα to signal lattice matrix, and an index, and then rα to signal coordinates?
According to the above, $\mathbf R$ would be a matrix, no?
That's true. Maybe we are trying to make these definitions to rigid?!
We could return to Nick's initial suggestion of indicating vectors with arrows. That would remove the ambiguity
What about avoiding $\mathbf R$?
$\mathbf r_i$ and $\mathbf rI$ could be vectors to an orbital and an atom, respectively? And $\mathbf r\alpha$ a cell vector?
What about avoiding R?
ri and rI could be vectors to an orbital and an atom, respectively? And rα a cell vector?
I think we can't really avoid $\mathbf R$ the arguments for gauge
is r
or R
, so I think it would be smart if we could retain that notation.
One final thing that occurred to me. Generally we have used R as lattice vectors, and r as atomic coordinates. So now, what should we do? Rα to signal lattice matrix, and an index, and then rα to signal coordinates?
According to the above, R would be a matrix, no?
But with indices $\mathbf R_\alpha$ it would be a column/vector for the matrix. We need to be able to use indices for matrices anyways, so I guess this is ok.
Otherwise, I think we really should go the $\vec R$ way.
What about reconsidering the inputs to gauge
? Maybe something like gauge='cell|orbital|atom'
would be more explicit than r
and R
?
indeed that would be a better choice. :)
Attention: Patch coverage is 68.33333%
with 38 lines
in your changes are missing coverage. Please review.
Project coverage is 86.78%. Comparing base (
e4a017b
) to head (0244d5c
).
Files | Patch % | Lines |
---|---|---|
src/sisl/physics/_ufuncs_matrix.py | 0.00% | 37 Missing :warning: |
src/sisl/viz/processors/wavefunction.py | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I have (in this pr) changed gauge=cell|orbital
for consistency.
The old R|r
is still accepted, without any deprecation warnings.
What about avoiding R?
ri and rI could be vectors to an orbital and an atom, respectively? And rα a cell vector?
I would still advocate we could use $\mathbf R_\alpha$ for indexing lattice vectors. Or, we should differentiate between indices, and labels.
For instance $\mathbf A\alpha$ will be the $\alpha$ column of the matrix. In this case that will result in a vector.
However, if you do $\mathbf a\alpha$ it will actually be a scalar. because you are taking out a single component.
So if we want to name a lattice vector with the $\alpha$ name we should do: $\mathbf r^\alpha$ or $\mathbf R_\alpha$, no?
I know I am being pedantic here. But it think this could be nice to enforce ;)
Upon reconsidering it, I think $\mathbf R_\alpha$ is a perfectly adequate way to name the lattice vectors. We can interpret $\mathbf R$ as a matrix where each column/row is a lattice vector.
Upon reconsidering it, I think Rα is a perfectly adequate way to name the lattice vectors. We can interpret R as a matrix where each column/row is a lattice vector.
OK, I like this too. So $\mathbf R$ would directly be the 3x3 matrix from Geometry.cell
.
For some generic coordinate vector $\mathbf r$ we could introduce labels as in $\mathbf r ^{(i)}$ or $\mathbf r ^{(I)}$, with parentheses to avoid interpretation as exponent.
I think I got pretty much everything now, probably a bit too fast sometimes ;)
@tfrederiksen @nils-wittemeier could you please have a look here.
Are there any things that is missing?
Once agreed upon, we should start migrating documentation to follow these guidelines. Preferably against this branch, so we solve everything in one go!
isort .
andblack .
[24.2.0] at top-leveldocs/
CHANGELOG.md