Skip to content

Issue #1346 - removed "get" from function names (except get_backend)#2516

Closed
TrishaPe wants to merge 3 commits into
Qiskit:masterfrom
TrishaPe:master
Closed

Issue #1346 - removed "get" from function names (except get_backend)#2516
TrishaPe wants to merge 3 commits into
Qiskit:masterfrom
TrishaPe:master

Conversation

@TrishaPe
Copy link
Copy Markdown
Contributor

Summary

Issue #1346 lists several function names with the word 'get' in the name, which doesn't do much to clarify what the function does (some are generating data, some are looking for data to store in variables). I changed their names, generally using 'generate', 'read' or 'return' instead.

Details and comments

DONE:

  • qiskit/qasm/_qasm.py:
    -- get_filename(self) to read_filename
    -- get_tokens(self): There was an instance of this in quasm.py and another in qasmparse.py with different programs attached to them - two functions with the same name and different contents. I changed the instance in qasm.py to generate_tokens and the one in qasmparse.py to read_tokens.

  • qiskit/result/_result.py:
    -- get_statevector(self, circuit=None): defined in result.py, referenced in 5 other files. I changed to return_statevector in all of them. There is still another function defined as _get_statevector in qasm_simulator.py, but this is a different one and only appears in this same file. It also has an underscore as the first character so I shouldn't touch it.
    -- get_counts to histogram_data

PROBLEMS:

  • qiskit/result/result.py:
    -- get_status(self), get_circuit_status(self, icircuit), get_job_id(self), get_ran_qasm(self, name), get_snapshots(self, circuit=None), get_snapshot(self, slot=None, circuit=None), get_names(self), get_qubitpol_vs_xval(self, nqubits, xvals_dict=None) do not exist
    -- get_data is referenced twice, but is not defined anywhere in the whole qiskit-terra folder. Maybe this text should be updated for clarity.

  • folder qiskit\backends has been turned into "providers"

  • qiskit\mapper does not exist

EXTRA REMARK:
I also tried to adapt 'get_backends', originally defined in baseprovider.py in qiskit/providers, but keep on receiving errors in the tests. I turned back all my changes and now the tests are fine, but this also means that this change still needs to be completed.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 26, 2019

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ajavadia ajavadia added the on hold Can not fix yet label Jun 2, 2019
@ajavadia
Copy link
Copy Markdown
Member

ajavadia commented Jun 2, 2019

I'm not sure we should do this. What is the problem with get_ to justify major API changes?

result.get_counts(), result.get_statevector(), result.get_unitary() are major APIs that have been in Qiskit for a long time now. I don't think we should change them.

I'm less concerned about the qasm changes.

@jaygambetta
Copy link
Copy Markdown
Member

It’s because we never closed the issue. We decided to keep the get in results class as they change the data. I think we need to close and update the issue

@ajavadia
Copy link
Copy Markdown
Member

ajavadia commented Jun 2, 2019

Yes I agree. @TrishaPe sorry that the issue you worked on was not updated. Do you mind making this PR only change the qasm functions?

@jaygambetta
Copy link
Copy Markdown
Member

I can fix the issue later

@ajavadia
Copy link
Copy Markdown
Member

@TrishaPe please let us know if you are still going to work on this

@1ucian0
Copy link
Copy Markdown
Member

1ucian0 commented Jun 24, 2019

This PR is on a master branch, so I dont know how to change it "in place". I created #2683 to keep the @TrishaPe authorship in her commits. So, I'm closing this one in favor of #2683

@1ucian0 1ucian0 closed this Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on hold Can not fix yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants