Skip to content
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

**Breaking**: data_kind: data is None and required now returns the 'empty' kind #3482

Merged
merged 30 commits into from
Oct 16, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Oct 3, 2024

Need to wait for #3351 or merge directly into #3351.

This PR adds a new kind "empty" for the data=None, required=True case. The new kind name makes more sense and can simplify the logic of codes.

@@ -1791,6 +1791,7 @@ def virtualfile_in( # noqa: PLR0912
"image": tempfile_from_image,
"stringio": self.virtualfile_from_stringio,
"matrix": self.virtualfile_from_matrix,
"none": self.virtualfile_from_vectors,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change to another word like "empty" to reduce confusion with None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another idea to completely eliminate the "none" kind. I'll open a POC PR later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another idea to completely eliminate the "none" kind. I'll open a POC PR later.

It turns out the idea can't eliminate the "none" kind. So let's focus on this PR again.

Can we change to another word like "empty" to reduce confusion with None?

I want to emphasize that data_kind(data=None) returns "none", so kind == "none" means data is None, which IMHO is very intuitive. That's why I choose "none" as the new kind name, but I'm also OK with a new kind name if others think "empty" is a better name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of kind == "none" is to indicate that the data is stored in the x, y, z variables rather than in data (i.e. data is None means data is in xyz variables). I thought about naming it as kind="xyz" but since we are planning to support other 1d arrays like intensity, symbol, etc (#1076), maybe we should call it kind="xyzarrays"? Or I'm ok with using kind="empty" too, just don't want to use "none" because it is confusing with None.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should call it kind="xyzarrays"

Although not documented, I think in our codes, when we talk about "arrays", we usually mean numpy arrays, so a better name is "xyzvectors", but I feel it's confusing with the "vectors" kind. So, rename none to empty in a1e67d3.

@seisman seisman added this to the 0.14.0 milestone Oct 8, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label Oct 8, 2024
@seisman seisman marked this pull request as ready for review October 8, 2024 04:01
@seisman seisman changed the title **Breaking**: data_kind: data is None and required now returns the 'none' kind **Breaking**: data_kind: data is None and required now returns the 'empty' kind Oct 11, 2024
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, but I think wait for #3351 to be merged first before merging this one.

pygmt/helpers/utils.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Oct 13, 2024
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 15, 2024
Base automatically changed from refactor/data_kind to main October 16, 2024 02:02
@seisman seisman force-pushed the data_kind/vectors-none branch from 80c5aa5 to a469acf Compare October 16, 2024 02:16
@seisman seisman merged commit d41922c into main Oct 16, 2024
21 checks passed
@seisman seisman deleted the data_kind/vectors-none branch October 16, 2024 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants