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

Code duplications in predictor.py #508

Closed
elcorto opened this issue Mar 26, 2024 · 2 comments
Closed

Code duplications in predictor.py #508

elcorto opened this issue Mar 26, 2024 · 2 comments

Comments

@elcorto
Copy link
Member

elcorto commented Mar 26, 2024

self.data.grid_dimension = self.parameters.inference_data_grid
self.data.grid_size = self.data.grid_dimension[0] * \
self.data.grid_dimension[1] * \
self.data.grid_dimension[2]
in __init__ is the same as
self.data.grid_dimension = self.parameters.inference_data_grid
self.data.grid_size = self.data.grid_dimension[0] * \
self.data.grid_dimension[1] * \
self.data.grid_dimension[2]
in predict_from_qeout(). Since the former is in the constructor, I think the latter can be skipped.

Also here

if np.prod(self.parameters.inference_data_grid) > 0:
self.data.grid_dimension = self.parameters.inference_data_grid
else:
# We need to check if we're in size transfer mode.
old_cell = self.data.target_calculator.atoms.get_cell()
new_cell = atoms.get_cell()
self.data.grid_dimension = self.parameters.inference_data_grid is already performed in __init__ so the check could be changed to something like

if np.prod(self.parameters.inference_data_grid) == 0:
    ... code from else clause here ...

Note: <= 0 would be correct but I gess np.prod(self.parameters.inference_data_grid) is never < 0.

@RandomDefaultUser
Copy link
Member

Hi @elcorto thanks for bringing this to my attention, I will have a look! :)

@RandomDefaultUser
Copy link
Member

I looked into the second part of this issue, and do no longer think this part of the code is redundant. The additional check makes sure that if the user specified an inference grid AFTER loading of the predictor class, this change is taken into account. Since the predictor object is loaded and the parameters object is not created beforehand, if you want to use a different inference grid this is the only way (I think?).

The first part is fixed by #562, so I am closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants