-
Notifications
You must be signed in to change notification settings - Fork 15
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
Laplacian method for Species class #131
Conversation
…dataset compilation script.
atomdb/datasets/numeric/run.py
Outdated
|
||
from atomdb.periodic import Element | ||
|
||
|
||
def load_numerical_hf_data(): | ||
def load_numerical_hf_data(data_path=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not def load_numerical_hf_data(data_path=DEFAULT_DATAPATH):
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even more, I think this function should not have a default value. The only place where it is used inside run
(line 183), a value is passed to this argument. I think the function that could/should have a default value should be the run function (line 155).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I think I'll go along the lines of your first suggestion making the change to simply
def load_numerical_hf_data(data_path)
I wouldn't make the change to the run
function to keep consistency across all compilation scripts, and since the default path gets defined as kwd argument in the load
function from the species module:
Lines 782 to 790 in 511a702
def load( | |
elem, | |
charge, | |
mult, | |
nexc=0, | |
dataset=DEFAULT_DATASET, | |
datapath=DEFAULT_DATAPATH, | |
remotepath=DEFAULT_REMOTE, | |
): |
The way I see it, the
run
functions will hardly ever get used directly by the user, so having the default value of datapath only in load
(and compile
) seems OK to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me. If @marco-2023 and @gabrielasd are happy with it's fine to merge IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good
This pull request adds support for computing the Laplacian of the electron density and fixes incorrectly assigned second derivative of density values in the numeric dataset compilation script.
Main changes:
dd_dens_lapl_func
is added to the Species class which (similar todens_func
) returns a callable function. The function is set to zero when evaluated at