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

mnt: changed only= keyword arguments to what= #542

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Feb 27, 2023

This is to unify interfaces that has these kinds of arguments. It affects Geometry|SuperCell.rotate and some tbtrans siles.

To accommodate backwards compatibility for some time I have added a function to decorate methods enabling a keyword argument and replacing the new keyword argument with the old keyword argument. I believe users who use the old argument are not aware of the new one and hence it makes more sense.
A deprecation warning is also issued.

The default for Geometry.rotate is now also changed depending on whether atoms argument is supplied.

  • atoms is None: what="xyz+abc"
  • else what="xyz" when doing sub-rotations it makes more sense to only rotate the atomic coordinates, while rotations of everything makes more sense to also rotate the lattice vectors.
  • Closes only or what? #541
  • Tests added
  • Documentation for functionality
  • User visible changes (including notable bug fixes) are documented in CHANGELOG
    @tfrederiksen could you please have a look whether this is ok? :)

This is to unify interfaces that has these kinds of arguments.
It affects Geometry|SuperCell.rotate and some tbtrans siles.

To accommodate backwards compatibility for some time I have
added a function to decorate methods enabling a keyword argument
and replacing the *new* keyword argument with the old keyword argument.
I believe users who use the old argument are not aware of the new one
and hence it makes more sense.
A deprecation warning is also issued.

The default for Geometry.rotate is now also changed depending on
whether atoms argument is supplied.
- atoms is None: what="xyz+abc"
- else what="xyz"
when doing sub-rotations it makes more sense to only rotate the
atomic coordinates, while rotations of everything makes more sense
to also rotate the lattice vectors.

Signed-off-by: Nick Papior <[email protected]>
@@ -366,7 +366,7 @@
@pytest.mark.filterwarnings("ignore", message="*gridncSileSiesta.read_grid cannot determine")
@pytest.mark.parametrize("sile", _my_intersect(["read_grid"], ["write_grid"]))
def test_read_write_grid(self, sisl_tmp, sisl_system, sile):
g = sisl_system.g.rotate(-30, sisl_system.g.cell[2, :])
g = sisl_system.g.rotate(-30, sisl_system.g.cell[2, :], what="xyz+abc")

Check warning

Code scanning / CodeQL

Variable defined multiple times

This assignment to 'g' is unnecessary as it is [redefined](1) before this value is used. This assignment to 'g' is unnecessary as it is [redefined](2) before this value is used.
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #542 (36c069b) into main (0d78d3d) will increase coverage by 0.00%.
The diff coverage is 92.82%.

❗ Current head 36c069b differs from pull request most recent head a61ee95. Consider uploading reports for the commit a61ee95 to get more accurate results

@@           Coverage Diff            @@
##             main     #542    +/-   ##
========================================
  Coverage   86.25%   86.26%            
========================================
  Files         362      362            
  Lines       46289    46409   +120     
========================================
+ Hits        39928    40033   +105     
- Misses       6361     6376    +15     
Impacted Files Coverage Δ
sisl/utils/misc.py 86.20% <ø> (ø)
sisl/tests/test_sgeom.py 96.55% <75.00%> (+0.06%) ⬆️
sisl/supercell.py 94.95% <84.61%> (-1.17%) ⬇️
sisl/io/tbtrans/tbt.py 78.11% <90.00%> (+0.12%) ⬆️
sisl/geometry.py 86.54% <90.38%> (-0.01%) ⬇️
sisl/geom/bilayer.py 98.66% <100.00%> (ø)
sisl/geom/nanoribbon.py 100.00% <100.00%> (ø)
sisl/io/tbtrans/tests/test_tbt.py 100.00% <100.00%> (ø)
sisl/io/tests/test_object.py 91.04% <100.00%> (ø)
sisl/messages.py 70.40% <100.00%> (+3.73%) ⬆️
... and 4 more

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

Copy link
Contributor

@tfrederiksen tfrederiksen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

sisl/geometry.py Outdated Show resolved Hide resolved
sisl/geometry.py Show resolved Hide resolved
@zerothi
Copy link
Owner Author

zerothi commented Feb 27, 2023

Thanks @tfrederiksen

@zerothi zerothi merged commit d46668e into main Feb 27, 2023
@zerothi zerothi deleted the 541-argument-what-unification branch February 27, 2023 13:03
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.

only or what?
2 participants