Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
pre-PR for util for condensation #449
pre-PR for util for condensation #449
Changes from 4 commits
364e421
97ddbb8
e801e35
06565e1
d929a48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually giving the users flexibility here? Or are we letting them walk into a trap? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reference to the
e.g.,
which implies other options are validThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 unitsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.