search

ACEsuit / ACE.jl

Parameterisation of Equivariant Properties of Particle Systems
65 stars 15 forks source link

WIP: isrealB / AA interface #51

Closed cortner closed 3 years ago

cortner commented 3 years ago

@MatthiasSachs @zhanglw0521 - tests are not passing yet, but I may not be able to finish debugging, so here is a first draft.

I've kept the isreal flag after all for a few reasons: (1) maybe we want the option to overwrite it; (2) it means it gets a lot harder to put something other than real or identity in there; and (3) because this is also in valtype it gets harder to dispatch the allocations correctly. Please look at the changes and see if you spot any issues with what I'm proposing now.

cortner commented 3 years ago

@MatthiasSachs and @zhanglw0521 I'd be grateful if you take a quick look at those changes - I'd like to merge sometime tomorrow.

cortner commented 3 years ago

@MatthiasSachs did you push your commits into this branch? I don't see them?

MatthiasSachs commented 3 years ago

Sorry, just pushed the changes. I wanted to rerun the test. But somehow it took too long to load some functions. If it doesn't pass the tests here I will take another look. Certainly, there some question about whether or not the new implementation is more intuitive. For example, the functions real and complex are now implemented as follows:

real(φ::EuclideanVector) = EuclideanVector(φ.val) complex(φ::EuclideanVector) = EuclideanVector(φ.val)

cortner commented 3 years ago

@MatthiasSachs Can you please configure your editor so that it doesn't add / remove arbitrary whitespace? This makes it really difficult to review the changes.

cortner commented 3 years ago

I'm happy and will merge now. If bugs emerge we deal with them. The structure is more important right now.

MatthiasSachs commented 3 years ago

@MatthiasSachs Can you please configure your editor so that it doesn't add / remove arbitrary whitespace? This makes it really difficult to review the changes.

Ok, I just modified the whitespace options in Atoms. I hope that will do the job.