pre-PR for util for condensation #449
Conversation
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Gorkowski - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| - mass_concentrations: A list or ndarray of mass concentrations | ||
| (e.g., kg/m^3). | ||
| - molar_masses: A list or ndarray of molecular weights (e.g., kg/mol). |
There was a problem hiding this comment.
Are we actually giving the users flexibility here? Or are we letting them walk into a trap? 😉
There was a problem hiding this comment.
In reference to the e.g., which implies other options are valid
There was a problem hiding this comment.
well, other units are possible if they use this in isolation, then that is the users problem. But, the e.g. was to say how next uses it, and give the base SI units
| if np.any(mass_concentrations < 0): | ||
| raise ValueError("Mass concentrations must be positive") | ||
| if np.any(molar_masses <= 0): | ||
| raise ValueError("Molar masses must be non-zero, positive") |
There was a problem hiding this comment.
Is it possible to have these be part of the typing? I actually don't know if that's a thing.
In general though, we are repeating a lot of code here, so a good place to consider refactoring. Would a random utility like this work? We can refine it later to include more detailed logging... speaking of which, we need to have al logger, but that's a whole different devops task for me to take on later....
import numpy as np
def enforce_nonneg(*args, where="someplace"):
for some in args:
if np.any(some <= 0):
raise ValueError(
f"{where}: must be positive, but got {some} ..."
)
# enforce_nonneg(1, -5, 6, where="somewhere")There was a problem hiding this comment.
Also, we need to put a lower limit on numpy to use these new features: https://numpy.org/devdocs/reference/typing.html#typing-numpy-typing (I can take care of that later)
There was a problem hiding this comment.
logger, yes. As for the abstract, I agree it should be done. It wasn't obvious how many spots in the code this should be done. I think through the PRs for condensation, the abstraction format (how many different checks need/should be done and how similar are they) and usage would become obvious.
There was a problem hiding this comment.
oh, as part of the typing... that sounds cool, but my preference is to stick with standard typing (flaots, np.ndarrays) to help others on board with the code.
Pre-PR with utility changes for condensation PR.
Create a sub util.converting for converting mass_concentration to different representations. This could be used to spread out the current convert.py collection.
Added unitless Knudsen number and mean free path of molecules