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

Apparent error in docstring for Operator.__init__ #335

Closed
bwohlberg opened this issue Sep 7, 2022 · 2 comments · Fixed by #336
Closed

Apparent error in docstring for Operator.__init__ #335

bwohlberg opened this issue Sep 7, 2022 · 2 comments · Fixed by #336
Labels
documentation Improvements or additions to documentation

Comments

@bwohlberg
Copy link
Collaborator

There appears to be an error in the docstring for Operator.__init__ :

eval_fn: Function used in evaluating this :class:`.Operator`.
Defaults to ``None``. If ``None``, then `self.__call__`
must be defined in any derived classes.

If I'm not mistaken, it should state "... then self._eval must be defined ...".

@Michael-T-McCann: do you agree?

@bwohlberg bwohlberg added the documentation Improvements or additions to documentation label Sep 7, 2022
@Michael-T-McCann
Copy link
Contributor

@Michael-T-McCann: do you agree?

Agreed.

Even with that fixed, I find it mightily confusing. Perhaps "Required unless __init__ is being called from a derived class with an _eval method." Whether this is a good design is a separate question.

Another positive change we could make: H = Operator((10,)) currently throws the error AttributeError: 'Operator' object has no attribute '_eval'. Instead, __init__ could check if eval_fn is not passed and self._eval is not defined and throw an error.

@bwohlberg
Copy link
Collaborator Author

Even with that fixed, I find it mightily confusing. Perhaps "Required unless __init__ is being called from a derived class with an _eval method."

That's quite a bit better.

Whether this is a good design is a separate question.

Agreed, but I'm inclined to leave that issue alone for now.

Another positive change we could make: H = Operator((10,)) currently throws the error AttributeError: 'Operator' object has no attribute '_eval'. Instead, __init__ could check if eval_fn is not passed and self._eval is not defined and throw an error.

I was thinking of that too. Will do.

bwohlberg added a commit that referenced this issue Sep 7, 2022
@bwohlberg bwohlberg mentioned this issue Sep 7, 2022
bwohlberg added a commit that referenced this issue Sep 7, 2022
* Resolve #335

* Clean up after black

* Improve docs

* Minor docs fix

* More minor docs improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants