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

Naming of Array.spox_var() and ndonnx.from_spox_var #75

Open
cbourjau opened this issue Sep 23, 2024 · 3 comments
Open

Naming of Array.spox_var() and ndonnx.from_spox_var #75

cbourjau opened this issue Sep 23, 2024 · 3 comments

Comments

@cbourjau
Copy link
Collaborator

We have had discussions recently about the naming of these functions. I believe there are two parts to consider here: the prefix and the suffix part.

Concerning the prefix, it might be valuable to harmonize the naming with the common unwrap prefix used in spox (e.g., Var.unwrap_tensor), which signals the fallibility nicely, IMHO. Concerning the suffix we may consider using only spox or var in the naming. Thus, I see the following options for the unwrap part:

  • unwrap_var
  • unwrap_spox
  • unwrap_spox_var
    and one of the following constructor functions:
  • from_var
  • from_spox
  • from_spox_var

I'd be in favor of unwrap_spox_var and from_spox_var. Any thoughts, @adityagoel4512 @neNasko1

@adityagoel4512
Copy link
Member

adityagoel4512 commented Sep 23, 2024

The fallibility of spox_var is documented in the docstring although I agree with you saying that it might be clearer in unwrap_spox_var. I'm not so sure that this is a good enough reason for a breaking change given it is already clearly documented however. The ndonnx-Spox interoperability is used extensively in a lot of places so making the name nicer would have to be traded off against the churn required in dependent projects. Are there any other reasons changing the name is valuable to you?

@cbourjau
Copy link
Collaborator Author

We would deprecate the existing function for a while of course.

@adityagoel4512
Copy link
Member

We should take advantage of not being 1.x and do this. Let's not give ourselves a strong deadline to remove spox_var but unwrap_spox_var is good to me 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants