bugfixes for element dependent cutoffs

[JPDarby]

Linked to ACEsuit/ACE1.jl#82

• passed element dependent cutoffs to multitransform so it doesn't just default to using the largest one
• sorted elements because PolyPairBasis seems to expect this

[cortner]

@JPDarby -- thanks for this. Do you agree this is a breaking change? There is probably a reason why we didn't do this in the first place.

@wcwitt and @CheukHinHoJerry can you please take a brief look and tell me if this has unforeseen consequences?

[JPDarby]

Just added a fix for manually specifying constant cutoff.

@cortner This certainly breaks the tests - which I think is expected because it looks like it's checking the final basis hasn't changed and it has because of the different cutoffs. Afraid I'm not sure what else could be affected

[cortner]

I have a suggestion how to keep this backward compatible. Why not keep the standard behaviour but add a kwarg that changes to the new behaviour that you are now proposing. Then we can test this for a while before switching to this as the default.

[JPDarby]

Good idea, have added one

[cortner]

So if I understood this correctly, you still keep a single rcut = max rcut(zi, zj) but you do allow species-dependent envelopes and this is now made possible by extending the envelopes by 0 outside the rcut(zi,zj). And further, this behaviour is turned on via the kwarg variable_cutoff = true.

Is this a descriptive summary of this PR together with ACE1.jl:#82 ?

[cortner]

And finally, do we agree that this does not break any public API for ACE1x (which isn't defined anyhow) due to the new kwarg. So if this PR breaks ACEpotentials then there is a bug to be fixed there.

[JPDarby]

So the changes affect the pair basis and the ACEbasis in different ways.

The pair basis already had element dependent radial transforms and looked like it supported element dependent cutoffs but ACE1.PolyEnvelope was returning non-zero values for r > rcut. The PR to ACE1 fixed this so this behaviour has changed in ACE1x.

Separately, within ACE1x the ACE basis already supported element dependent radial transforms and again, looked like it supported element dependent cutoffs. The variable cutoffs weren't working for two reasons:

1. the variable cutoffs were not being passed to multitransform (ACE1.PolyEnvelope is not used here)
2. the element were not sorted, as expected by PolyPairBasis, so the wrong cutoff was being used for the wrong element.

This PR fixes (I think) 1. and 2. if variable_cutoff= true

[JPDarby]

And finally, do we agree that this does not break any public API for ACE1x (which isn't defined anyhow) due to the new kwarg. So if this PR breaks ACEpotentials then there is a bug to be fixed there.

So the default behaviour of the pair basis in ACEPotentials.jl will have changed because of the PR to ACE1

[cortner]

I thought that was a bugfix rather than a change in behaviour. We should test that.

[JPDarby]

Maybe I'm using "change in behaviour" incorrectly. I agree it was a bug!

[cortner]

Basically the point is that if a downstream package relies on internal undocumented behaviour then there is guarantee

[wcwitt]

Sorry for the slow reply - is this ready for me to look closely now?

[JPDarby]

@wcwitt yes please that would be great

[CheukHinHoJerry]

@JPDarby -- thanks for this. Do you agree this is a breaking change? There is probably a reason why we didn't do this in the first place.

@wcwitt and @CheukHinHoJerry can you please take a brief look and tell me if this has unforeseen consequences?

Sorry for late reply - will look into this closely tonight.

[wcwitt]

Why not keep the standard behaviour but add a kwarg that changes to the new behaviour that you are now proposing. Then we can test this for a while before switching to this as the default.

Since this does seem to be a bug, or at least unintended behavior, would you be opposed to merging and tagging (the tests will still pass), then immediately tagging a new breaking change version where the kwarg default switches to the new behavior? The tests would need updating in that case.

[cortner]

My understanding is that the bugfix is alreay taken care of. If nobody has concerns about THIS PR being breaking then I will merge and tag it as well.

[cortner]

But I am not convinced yet that the "new behaviour" should automatically become the default.

[CheukHinHoJerry]

I agree with merging this PR, test for a while before we switch to a new default. This is something too subtle that other users might not even be aware of. Maybe we can then have another PR that has a new default with test updated and we report issues and discuss there?

[JPDarby]

Sorry for slow reply, was away at the end of last week. Yes I agree with merging this PR as is and testing the new behaviour for a while.

[cortner]

this should now be available under [email protected].