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

Should spox.opset.ai.onnx.vXX.const default to float64 for float input? #24

Closed
cbourjau opened this issue Feb 7, 2023 · 8 comments · Fixed by #45
Closed

Should spox.opset.ai.onnx.vXX.const default to float64 for float input? #24

cbourjau opened this issue Feb 7, 2023 · 8 comments · Fixed by #45
Labels
breaking Might cause breaking changes opset Opset generation & modules stable Related to the stable public interface
Milestone

Comments

@cbourjau
Copy link
Collaborator

cbourjau commented Feb 7, 2023

Numpy defaults to float64 when presented with python floats:

np.array([3.14]).dtype == np.dtype("float64")

However, the const convenience method defaults to float32. We should consider changing this behavior in order to stay closer to numpy's behavior.

@JakubBachurskiQC
Copy link
Contributor

This is indeed a good question. We did end up using npt.DTypeLike in cast which made op.cast(x, to=float) cast to float64. I could see that being an argument for op.const(x: float) becoming float64.
It would be however unfortunate to isolate ML/DL-oriented users that would prefer float32-oriented behaviour. I'm not sure what would be the best way of approaching this.

@JakubBachurskiQC
Copy link
Contributor

I would propose changing semantics of const to arg -> op.constant(value=numpy.array(arg)), or removing it entirely.
It's not a battle worth fighting further by trying to come up with conventions! Thoughts @cbourjau ?

@cbourjau
Copy link
Collaborator Author

I think having const is very nice. op.constant(value=np.array(42)) is certainly less nice to type out than op.const(42). Delegating it all to numpy, as you suggest, seems to be the easier and right thing to do, though!

@JakubBachurskiQC
Copy link
Contributor

Using an equivalent of this I think the only thing that was missing was having a dtype argument, defaulting to what numpy.array does when none is provided.

@cbourjau
Copy link
Collaborator Author

I'm not sure I understand what you mean. Are you saying we should have something like op.const(arg, dtype=np.float32)?

@JakubBachurskiQC
Copy link
Contributor

Yes - as otherwise op.const(3.14) is always an f64. Writing op.const(np.array(3.14, np.float32)) is a bit longer than it could be: op.const(3.14, np.float32).

@cbourjau
Copy link
Collaborator Author

Yes, that makes sense!

@cbourjau cbourjau transferred this issue from another repository Feb 20, 2023
@JakubBachurskiQC JakubBachurskiQC added stable Related to the stable public interface opset Opset generation & modules and removed enhancement labels Feb 23, 2023
@JakubBachurskiQC
Copy link
Contributor

I'm debating whether we should be using Constant for this at all. It seems that initialisers are the actual expected mechanism for constants on this IR version. We'll get to this issue if we want to do #25 - as other opset modules (well, only ai.onnx.ml currently) will also want to construct constants. So maybe the actual best approach is to expose a spox.const|initializer (ArrayLike) -> Var public function that uses initialisers underneath?

@JakubBachurskiQC JakubBachurskiQC added this to the 1.0 milestone Feb 27, 2023
@JakubBachurskiQC JakubBachurskiQC added the breaking Might cause breaking changes label Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Might cause breaking changes opset Opset generation & modules stable Related to the stable public interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants