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

Extended rzz validation to include parameter expressions #2093

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yaelbh
Copy link
Collaborator

@yaelbh yaelbh commented Jan 2, 2025

Summary

Details and comments

Fixes #2067

@yaelbh yaelbh requested a review from ihincks January 2, 2025 12:28
@yaelbh yaelbh requested a review from wshanks January 13, 2025 09:05
Copy link
Collaborator

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I think a release note would be good since it is a user-facing change (in the sense of the user getting an error in the client instead of an error from the service after a bad parameter binding).

I was not familiar with the internals of the Pub classes before so reading this code I was concerned about whether it handled multiple dimensions correctly. I added some dimensions to the tests and confirmed that it did.

The part that made me worried about dimensions (not being that familiar with the pub classes) was the relationship between list(chain.from_iterable(pub.parameter_values.data)) and pub.parameter_values.ravel().as_array(). It might be nice if the pub class had a parameter_names property that was a shorthand for the first expression and if as_array documented that its columns matched parameter_names when its parameters argument was not used so that the connection was more clear.

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

Successfully merging this pull request may close these issues.

Validate parameter expressions in rzz
2 participants