Clean up a bit using new dependencies #288
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. @@ Coverage Diff @@
## master #288 +/- ##
==========================================
+ Coverage 95.79% 95.85% +0.06%
==========================================
Files 28 28
Lines 3992 3983 -9
Branches 803 803
==========================================
- Hits 3824 3818 -6
+ Misses 99 97 -2
+ Partials 69 68 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I have used the Literal type annotation everywhere I could find strings being used to switch between a few options. Another possible tool for this kind of thing is Enum. Python 3.11 introduces the StrEnum which could be especially nice for doing this in a backward-compatible way. |
|
I'm baffled by these codacy errors. Those items are clearly type-hinted as Quantity. They assert as Quantity. mypy doesn't complain about them. Why is Codacy convinced they are numpy arrays? (mypy doesn't like that they are indexed, though. Which is a bit strange; how does it complain that it doesn't have stubs for pint, but also that it doesn't think Quantity should be indexable?) |
|
I think the .pylintrc has to be pushed to master branch before it will be used by Codacy :-( |
Using some features of Python 3.8+ and newer Numpy I tried not to go too crazy, there are more places where assignment expressions (i.e. walrus) could be used to save a line but overall flow and clarity don't really benefit.
Newer versions of Pint and Numpy allow a lot more Numpy operations to "just work" so we can cut a lot of boilerplate conversions. Unfortunate np.linalg.norm is not one of these yet! For now we'll leave the casting on that one, but np.square, np.sqrt and np.sum are ok...
There are lots of places in spectra where only a few specific strings are allowed. This can be hinted clearly with the Literal type from Python 3.8+ An even stricter implementation would use Enums, but making this backward-compatible could be clunky until the Python 3.11 StrEnum is available. Importing Quantity from Pint rather than indirectly from Euphonic seems to make mypy slightly less grumpy.
Codacy complains that at this line "Instance of ndarray has no 'to' member" But it should always be a Quantity? Try inserting assertion and see what happens. (Overly long line complaint is fair enough...)
Now that Quantities are handled a bit better, it is really only "needed" in force_constants... And it's not good practice to use private methods from other modules anyway. It doesn't hurt to make this casting to Bohr more explicit anyway, it makes it clearer that this part of the maths is done in atomic units without Pint wrappers.
ajjackson
force-pushed
the
embrace_the_walrus
branch
from
c39e523 to
5b66185
Compare
|
That seems to have fixed it 🎉 . I had to also edit the codacy settings to look for a pylintrc file. |
| @@ -1097,7 +1099,7 @@ def _dipole_correction_init(crystal: Crystal, | |||
| recip_q0 += recip_q0_tmp | |||
| else: | |||
| break | |||
| vol = crystal._cell_volume() | |||
| vol = crystal.cell_volume().to('bohr^3').magnitude | |||
There was a problem hiding this comment.
What is "magnitude" doing here? Is it a pint thing to return sans unit?
There was a problem hiding this comment.
Exactly, magnitude unwraps it to a bare array. This is why we usually chain it with to so we know what the unit will be and can use the values (relatively) safely.
The implementation is case-insensitive, this just makes the code more consistent to read. Existing scripts and tests should be unaffected
With a little wrapper we can have a nice for-loop which is more readable.
Co-authored-by: Jacob Wilkins <[email protected]>
Using some features of Python 3.8+ and newer Numpy
I tried not to go too crazy, there are more places where assignment expressions (i.e. walrus) could be used to save a line but overall flow and clarity don't really benefit.