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

Remove discussion of addresses from API and docs and replace with "CI strings" #54

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

caleb-johnson
Copy link
Collaborator

@caleb-johnson caleb-johnson commented Sep 20, 2024

The term "address", as used by PySCF, was being incorrectly used throughout the API and docs. Let's revert that discussion and use the correct/consistent-with-pyscf term, "CI strings".

  • Release note

@jrm874
Copy link
Collaborator

jrm874 commented Sep 20, 2024

@caleb-johnson , should we also change the name of variables?

@caleb-johnson
Copy link
Collaborator Author

@caleb-johnson , should we also change the name of variables?

Yes, I believe I got them all. Please let me know if you see any I missed

@@ -193,20 +192,20 @@ def optimize_orbitals(
"""
if isinstance(bitstring_matrix, tuple):
warnings.warn(
"Passing a length-2 tuple of sorted addresses to define the subspace is deprecated. Users "
"Passing a length-2 tuple of base-10 determinants to define the subspace is deprecated. Users "
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think simply "determinants" is more accurate than "base-10 determinants." They are simply integers, without reference to a "base."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the warning message

bitstring_matrix: np.ndarray, open_shell: bool = False
) -> tuple[np.ndarray, np.ndarray]:
"""
Convert bitstrings (rows) in a ``bitstring_matrix`` into base-10 integer representations of determinants.
Copy link
Collaborator

Choose a reason for hiding this comment

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

They're just integers, not "base-10" integers.

Copy link
Collaborator Author

@caleb-johnson caleb-johnson Sep 20, 2024

Choose a reason for hiding this comment

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

I started thinking that integers can be specified as base-2, which is a pretty natural way to specify these configurations as well. So I thought being explicit wouldn't hurt. I guess saying "integer" in a software context implies base-10

Copy link
Collaborator

Choose a reason for hiding this comment

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

The integers can be specified in base-2, or any other base. That's my point. The base 10 is not special here, nor in a general software context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed all base-10 mentions in fermion module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, on further thought, base 2 is special here, because the integer representation of a determinant is obtained by converting its string representation to an integer, in base 2.

@kevinsung
Copy link
Collaborator

kevinsung commented Sep 20, 2024

I have a separate general comment, which is that I think calling integers "base-10 integers" is confusing. They're just integers. Not sure if we want to fix that here or in a separate PR.

@caleb-johnson
Copy link
Collaborator Author

caleb-johnson commented Sep 20, 2024

I have a separate general comment, which is that I think calling integers "base-10 integers" is confusing. They're just integers. Not sure if we want to fix that here or in a separate PR.

Ya I commented on this above. A base-2 integer is also an integer, and it's a natural structure in this context, so I wanted to be explicit. If you think it just adds confusion or is unnecessary. I've removed all of that from the fermion module. Can clean it up elsewhere in another PR

@kevinsung
Copy link
Collaborator

I have a separate general comment, which is that I think calling integers "base-10 integers" is confusing. They're just integers. Not sure if we want to fix that here or in a separate PR.

Ya I commented on this above. A base-2 integer is also an integer, and it's a natural structure in this context, so I wanted to be explicit. If you think it just adds confusion or is unnecessary. I've removed all of that from the fermion module. Can clean it up elsewhere in another PR

Alright as we discussed offline, there is no such thing as a "base-2 integer" or a "base-10 integer," only "integers." I'm fine with fixing the terminology in a separate PR. I've opened #57.

@caleb-johnson caleb-johnson merged commit 0819a31 into main Sep 23, 2024
9 checks passed
@caleb-johnson caleb-johnson deleted the deprecate-addr branch September 23, 2024 16:44
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.

3 participants