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

EDA Functionality for LASSI-PDFT #121

Merged
merged 10 commits into from
Sep 10, 2024
Merged

Conversation

JangidBhavnesh
Copy link
Contributor

I have added / pointed the analyze and EDA functionality.

Related to #120

@JangidBhavnesh JangidBhavnesh changed the base branch from master to dev August 31, 2024 20:12
@MatthewRHermes
Copy link
Owner

If pyscf-forge #64 is accepted, will it be possible to simplify this? And can you add a unit test for it?

@JangidBhavnesh
Copy link
Contributor Author

If pyscf-forge #64 is accepted, will it be possible to simplify this? And can you add a unit test for it?

I have addressed these. Currently, I have to keep the duplicate of the function from the mcpdft to adopt the nroots as a list.

@MatthewRHermes
Copy link
Owner

MatthewRHermes commented Sep 10, 2024

If pyscf-forge #64 is accepted, will it be possible to simplify this? And can you add a unit test for it?

I have addressed these. Currently, I have to keep the duplicate of the function from the mcpdft to adopt the nroots as a list.

OK but ideally, we wouldn't change the datatype of mc.fcisolver.nroots because that may cause many versions of this problem down the road. A cleaner solution imo is to set mc.fcisolver.nroots to the length of the state list, store the state list somewhere else, and modify make_one_casdm* to index into the state list.

ETA: But I won't enforce this before accepting, just address my other comment about identifying sequences.

Made changes to detect the list.
@JangidBhavnesh
Copy link
Contributor Author

If pyscf-forge #64 is accepted, will it be possible to simplify this? And can you add a unit test for it?

I have addressed these. Currently, I have to keep the duplicate of the function from the mcpdft to adopt the nroots as a list.

OK but ideally, we wouldn't change the datatype of mc.fcisolver.nroots because that may cause many versions of this problem down the road. A cleaner solution imo is to set mc.fcisolver.nroots to the length of the state list, store the state list somewhere else, and modify make_one_casdm* to index into the state list.

ETA: But I won't enforce this before accepting, just address my other comment about identifying sequences.

Yes. I will make these changes before you accept this PR.

@MatthewRHermes
Copy link
Owner

Thanks!

@MatthewRHermes MatthewRHermes merged commit 48fea43 into MatthewRHermes:dev Sep 10, 2024
2 of 3 checks passed
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.

2 participants