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
Dataclass fixes #317
Dataclass fixes #317
Changes from 17 commits
ecb597d
623e5bc
7e5fe82
dd62506
9ab6a1e
9db217c
5ee0cdb
c25e23f
25bb06f
0223d67
cb31daf
a58eb5a
9b2cbbf
4487d3e
990edcf
df03c31
e64d304
3085b79
7b844bc
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.
I would change
ERROR
toWARNING
here, so that users realize that the ADF will keep going.The same request holds for the other warnings in this file as well.
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.
I think we probably just need to make this a bit more robust with something like
scale_factor = kwargs.get('scale_factor', 1)
add_offset = kwargs.get('add_offset', 0)
Just in case it gets used without the kwargs being passed.
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.
Actually, maybe use
get_defaults
to do this?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.
That's what I was attempting to do with
get_defaults
, it should be setting those to the defaults before it becomes thekwargs
I have the offset set to 0 and scale factor to 1 initially, then the check
if variablename in res
should grab the values and if not it will default to 0/1 respectively withvres.get
.And if the variable is not in
self.adf.variable_defaults
, the original values fornew_unit
,add_offset
, andscale_factor
will take on the original set values of none, 0, and 1.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.
I agree with @brianpm's original suggestion here. Right now if someone tried to use this function without passing in the
scale_factor
andadd_offset
keywords I believe the function would crash with a dictionary key error.