-
Notifications
You must be signed in to change notification settings - Fork 224
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
Allow passing None explicitly to pygmt functions Part 1 #1857
Changes from 14 commits
3cd0540
3b722e6
7b10bd2
2e58272
8440848
e1b71d2
2d78667
900273b
f1e8d37
8d8f138
58390de
bc09dcb
56fee1f
b7fd366
c7725e0
a8f5705
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -238,8 +238,12 @@ def velo(self, data=None, **kwargs): | |||||||||||||||
""" | ||||||||||||||||
kwargs = self._preprocess(**kwargs) # pylint: disable=protected-access | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really related to the PR, but it's something that I found confusing while working on part 2. Why is kwargs passed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, this ensures that Lines 109 to 115 in 0aa04d7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to me why we call the figure method to ensure the correct figure is activated in the session. I still don't really understand why we pass and return kwargs to/from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was setup for any other preprocessing of kwargs that may be needed or other figure plotting methods. At the time, I put this in place to allow things like passing It could also be useful for any bookkeeping operation that is needed, like activating the figure, activating a subplot, etc. |
||||||||||||||||
|
||||||||||||||||
if "S" not in kwargs or ("S" in kwargs and not isinstance(kwargs["S"], str)): | ||||||||||||||||
raise GMTInvalidInput("Spec is a required argument and has to be a string.") | ||||||||||||||||
if kwargs.get("S") is None or ( | ||||||||||||||||
kwargs.get("S") is not None and not isinstance(kwargs["S"], str) | ||||||||||||||||
): | ||||||||||||||||
raise GMTInvalidInput( | ||||||||||||||||
"The parameter `spec` is required and has to be a string." | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
if isinstance(data, np.ndarray) and not pd.api.types.is_numeric_dtype(data): | ||||||||||||||||
raise GMTInvalidInput( | ||||||||||||||||
|
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 this is slightly more readable (inspired by item number 16 in Effective Python (2021)).
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.
Ok, my brain is telling me that there's a reason not to do variable assignment like
a = b = "c"
, something about how changingb
would also changea
but after some testing, it seems to be ok. Maybe I'm just thinking of some other programming language I've used before like Matlab, Javascript or Python 2 🤷