search

ACEsuit / ACEHAL

11 stars 7 forks source link

ARD sklearn sigma fix, BIC ARD fix, dimer E0 fix #2

Closed casv2 closed 1 year ago

casv2 commented 1 year ago

This fixes the ARD sklearn sigma problem, looks for the BIC coefficients and fixes dimer plotting.

bernstei commented 1 year ago

Can you get rid of the irrelevant files (e.g. the stuff in build/, maybe other things)? And also add build to .gitignore? I thought I already added it there.

bernstei commented 1 year ago

Am I correct that now bases/default.py and bases/smooth.py are identical except for the smoothness prior? IF that's true, should there be just one option, with a hyper controlling the smoothness prior? We could extend it to different variants, too, e.g. the exponential that's being discussed in the slack.

casv2 commented 1 year ago

Am I correct that now bases/default.py and bases/smooth.py are identical except for the smoothness prior? IF that's true, should there be just one option, with a hyper controlling the smoothness prior? We could extend it to different variants, too, e.g. the exponential that's being discussed in the slack.

Yes this is the right way to go, we just keep the default and have options for the different smoothness priors

casv2 commented 1 year ago

I've only added none and algebraic, once the exponential becomes available we can add it. We can adjust the strength from the fixed_basis_params

casv2 commented 1 year ago

I should note I ran into a NaN running with the algebraic smoothness prior, but I think it's related something deep in ACE1.jl, I filed an issue there. Residual errors looked great in Python, so the fitting went smoothly

bernstei commented 1 year ago

@casv2 I merged the github CI pytest PR. Can you merge main into this one, so the tests run here as well?

casv2 commented 1 year ago

I've had to move back to the old sigma option as the proposed didn't work. Now there's an environment problem even it's the one I use. I'll investigate.

casv2 commented 1 year ago

It's a conflict because we need to be on ACE1x 0.0.4, but after adding ACE1pack it can only pull up to ACE1x 0.0.2 in the tests... I guess this needs to be resolved in ACE1pack. Instantiating from the (shipped) environment does seem to work