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

WIP: Sphericart integration #83

Closed
wants to merge 3 commits into from
Closed

WIP: Sphericart integration #83

wants to merge 3 commits into from

Conversation

CheukHinHoJerry
Copy link
Collaborator

@CheukHinHoJerry CheukHinHoJerry commented Jan 28, 2024

This PR aims at integrating SpheriCart https://github.com/lab-cosmo/sphericart to p4ml. Todo list that I come up with but feel free to update edit. This also fixes a bug that the current real solid harmonics returns NaN when an (approximately) zero vector is given or does not evaluate.

  • p4ml interface for real solid harmonics in SpheriCart
  • normalization of real solid harmonics in SpheriCart which compatible with current p4ml

If we want to retire the old backend entirely (though this sounds to me too ambitious):

  • make sure all the laplacian/derivative/pullback/pushforward of previous solid/spherical harmonics can be called exactly the same as before
  • check the normalization is correct for each basis, or provide an option normlization = :p4ml which recovers the previous normalization
  • and possibly evaluate the complex spherical harmonics via a transformation? This sounds something to be done in SpheriCart
  • update documentation, and maybe tag a minor version?

CC @zhanglw0521 @TSGut

@cortner
Copy link
Member

cortner commented Jan 28, 2024

If we want to retire the old interface entirely

What do you mean by "the old interface"? The interface should remain the same only the backend - i.e. the code that generates the basis will be sphericart?

@cortner
Copy link
Member

cortner commented Jan 28, 2024

I think all your points are very doable and should be done. Thank you for picking this up.

@CheukHinHoJerry
Copy link
Collaborator Author

If we want to retire the old interface entirely

What do you mean by "the old interface"? The interface should remain the same only the backend - i.e. the code that generates the basis will be sphericart?

Sorry that was a typo - yes I mean backend.

@CheukHinHoJerry
Copy link
Collaborator Author

Something like this should give a correct scaling up to a sign. This comes from the Condon-Shortley Sign Convention. Is there a better way to fix this other than modifying the sign at the end of the output directly?

function generate_Flms(L::Integer; normalisation = :p4ml, T = Float64)
   Flm = OffsetMatrix(zeros(L+1, L+1), (-1, -1))
   for l = 0:L
      Flm[l, 0] = sqrt(2)
      for m = 1:l 
         Flm[l, m] = Flm[l, m-1] / sqrt((l+m) * (l+1-m))
      end
   end
   return Flm
end

@cortner
Copy link
Member

cortner commented Feb 11, 2024

Are you saying that you need to rescale the Ylm output vector by a sign after the computation, and this cannot be done at the stage of precomputing the Flms?

@CheukHinHoJerry
Copy link
Collaborator Author

Are you saying that you need to rescale the Ylm output vector by a sign after the computation, and this cannot be done at the stage of precomputing the Flms?

Yes exactly.

@cortner
Copy link
Member

cortner commented Feb 11, 2024

this is unfortunate. But do we really care about the sign convention?

@cortner
Copy link
Member

cortner commented Feb 11, 2024

@DexuanZhou -- what do you think? Can you also ask Huajie? (i can't find his github name right now)

@hjchen1983
Copy link

@DexuanZhou -- what do you think? Can you also ask Huajie? (i can't find his github name right now)

Sorry I just saw this message. In our current simulation of Schrodinger equations, I think the sign convention is not a problem, we will be happy as long as it is a complete basis.

But I am not sure whether this could become an issue in future project. For example, when we consider some symmetry of the system and evaluate the Clebsch-Gordan coefficients?

@DexuanZhou
Copy link
Collaborator

DexuanZhou commented Feb 13, 2024

Using it as a one-particle basis function, I agree with Huajie that the sign convention shouldn't matter much. The only potential issue arises when we compare with chemical basis functions, where they might express one-particle basis as f = a_1f_1+a_2f_2+a_3f_3. In such cases, we need to be very careful to ensure that the coefficients align properly; for instance, adjusting coefficients to -a1, a2, and a3 as needed.

@DexuanZhou
Copy link
Collaborator

My point is, if the sign convention proves difficult to reconcile, we can compensate for it by adjusting the coefficients in the combination of basis functions. This ensures that the overall wave function aligns with the chemical basis functions for comparison purposes.

@cortner
Copy link
Member

cortner commented Feb 13, 2024

That sound to me like we should try to recover the correct signs. I'll discuss with Jerry.

@cortner
Copy link
Member

cortner commented Feb 13, 2024

... on the other hand we can just fix the signs in the clebsch-gordans ...

@cortner
Copy link
Member

cortner commented Jun 3, 2024

this PR seems to have gone stale. What shall we do?

@CheukHinHoJerry
Copy link
Collaborator Author

CheukHinHoJerry commented Jun 3, 2024

I remember there is a normalization to be fixed before merging, but for this reason this can only be done on Sphercart end.

Are you saying that you need to rescale the Ylm output vector by a sign after the computation, and this cannot be done at the stage of precomputing the Flms?

@cortner
Copy link
Member

cortner commented Jun 3, 2024

So exactly what is needed at the Sphericart end?

@CheukHinHoJerry
Copy link
Collaborator Author

CheukHinHoJerry commented Jun 3, 2024

We didn't arrive at a conclusion but we decide to make a issue on that end to discuss it (which I forgot to, sorry about that). I just skim over the code again, it make sense to me just to scale the output in P4ML?

@cortner
Copy link
Member

cortner commented Jun 3, 2024

Let's discuss at our next meeting what we should do.

@cortner
Copy link
Member

cortner commented Jun 7, 2024

Please double-check whether #84 makes this PR obsolete and if you agree, then please close it.

@CheukHinHoJerry CheukHinHoJerry deleted the scrrlm branch June 8, 2024 05:57
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.

4 participants