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

bug: fixed swapaxes handling #540

Merged
merged 1 commit into from
Feb 26, 2023
Merged

bug: fixed swapaxes handling #540

merged 1 commit into from
Feb 26, 2023

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Feb 11, 2023

Now the swapaxes is correct for Cartesian coordinate swaps.

Also extended it to be much more versatile.
One can do collapsed swappings, which might not be that useful. But rather easy to follow given that I wanted the str arguments.

Thanks to @ahkole for discovering the bug leading to this.

Signed-off-by: Nick Papior [email protected]

Now the swapaxes is correct for Cartesian coordinate swaps.

Also extended it to be much more versatile.
One can do collapsed swappings, which might not be that useful.
But rather easy to follow given that I wanted the str arguments.

Thanks to @ahkole for discovering the bug leading to this.

Signed-off-by: Nick Papior <[email protected]>
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #540 (6a273cb) into main (09674fc) will increase coverage by 0.03%.
The diff coverage is 96.06%.

@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
+ Coverage   86.24%   86.27%   +0.03%     
==========================================
  Files         361      362       +1     
  Lines       46253    46367     +114     
==========================================
+ Hits        39889    40002     +113     
- Misses       6364     6365       +1     
Impacted Files Coverage Δ
sisl/nodes/workflow.py 0.00% <0.00%> (ø)
sisl/supercell.py 95.26% <84.61%> (-0.86%) ⬇️
sisl/geometry.py 86.67% <100.00%> (+0.15%) ⬆️
sisl/mixing/base.py 93.67% <100.00%> (+0.42%) ⬆️
sisl/mixing/diis.py 96.05% <100.00%> (+0.10%) ⬆️
sisl/mixing/linear.py 66.66% <100.00%> (+1.80%) ⬆️
sisl/tests/test_geometry.py 99.67% <100.00%> (+<0.01%) ⬆️
sisl/tests/test_supercell.py 100.00% <100.00%> (ø)
sisl/utils/tests/test_arrays.py 100.00% <100.00%> (ø)
sisl/geom/nanotube.py 95.83% <0.00%> (+1.04%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zerothi
Copy link
Owner Author

zerothi commented Feb 12, 2023

@ahkole I don't know if the message pinged you or not, but here it is again :)

@zerothi
Copy link
Owner Author

zerothi commented Feb 26, 2023

@ahkole I will merge this as I think it is the correct thing, thanks for bringing this to my attention!

@zerothi zerothi merged commit 32328b5 into main Feb 26, 2023
@zerothi zerothi deleted the 539-swapaxes branch February 26, 2023 13:11
@ahkole
Copy link
Contributor

ahkole commented Mar 1, 2023

@zerothi Hi, sorry. I recently switched phones and haven't setup everything on my new phone yet (including the mail account where I receive GitHub notifications) so I completely missed this, sorry.

I had a look at the new implementation and as far as I can see all the swaps now work as I would expect. Maybe the only thing that could still be improved is the docstrings for the swapaxes method. First of all, inside the Geometry.swapaxes docstring there is this example,

g_s = g.swapaxes("abx", "bcz", 1, "xyz")

But this call should not be possible because swapaxes can get at most 3 arguments. Furthermore, what wasn't immediately clear to me from the docstring is that the what parameter is only used if you use integer arguments for axis_a and axis_b and also that using a string of labels instead of a single label for axis_a and axis_b means that multiple swaps will be executed in serial one after the other.

@zerothi
Copy link
Owner Author

zerothi commented Mar 1, 2023

@ahkole thanks for taking a look! The recent commit should be more verbose! Thanks for finding the doc bug ;)

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.

What should Geometry.swapaxes do if it includes xyz in what?
2 participants