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

VisibleDeprecationWarning in weight class #116

Open
ipcamit opened this issue Apr 2, 2023 · 4 comments
Open

VisibleDeprecationWarning in weight class #116

ipcamit opened this issue Apr 2, 2023 · 4 comments

Comments

@ipcamit
Copy link

ipcamit commented Apr 2, 2023

Line 177 in kliff.dataset.weight module is giving the following error in Python > 3.9, and error in Python >3.10

ValueError: setting an array element with a sequence. The requested array has an inhomogeneous shape after 1 dimensions. The detected shape was (2,) + inhomogeneous part.

originating here:

    def _compute_weight_one_property(data_norm, property_weight_params, property_type):
        c1, c2 = property_weight_params
->        sigma = np.linalg.norm([c1, (c2 * data_norm)])
        weight = 1 / sigma
...

@yonatank93 I am currently adapting it for new workflow, can you please tell what is the expected behavior of this, so I can modify it accordingly?

Does this captures your intent correctly? :

    c1_arr = np.ones_like(data_norm) * c1
    c2_arr = data_norm * c2
    sigma = np.linalg.norm(np.vstack((c1_arr, c2_arr)), axis=0) 

Both functions give identical results.

But as per the equation defined in MagnitudeInverseWeight here: https://kliff.readthedocs.io/en/latest/modules/dataset.html#magnitude-inverse-weight

I think it should be:

    sigma = np.linalg.norm([c1, c2 * np.linalg.norm(data_norm)]) 
@yonatank93
Copy link
Contributor

Hi @ipcamit.
I think we should modify L177 as

sigma = np.array([np.linalg.norm(c1, c2 * dn) for dn in data_norm])

The equation in the documentation is for per data point.
And, for example, if the configuration has 3 atoms in it, the function _compute_weight_one_property will return 3 numbers, one corresponding to each atom.

If we apply this change, I think we also need to apply additional minor change, for example to L146-L148

self._energy_weight = self._compute_weight_one_property(
        [energy_norm], self._weight_params["energy_weight_params"], "energy"
)[0]

and to L165-L167

self._stress_weight = self._compute_weight_one_property(
        [stress_norm], self._weight_params["stress_weight_params"], "stress"
)[0]

Note that I haven't tested these suggestions.

The reason is that for a configuration with N atoms, self._forces_weight needs to be a 1D array with 3N elements, and self._energy_weight and self._stress_weight each needs to be a float.

@yonatank93 yonatank93 mentioned this issue Apr 3, 2023
4 tasks
@mjwen
Copy link
Collaborator

mjwen commented Apr 4, 2023

@yonatank93 you seem updated the Weight class in the just merged PR. Is the issue reported by @ipcamit resolved there?

@yonatank93
Copy link
Contributor

Yes, I did. I think it is resolved.

@mjwen
Copy link
Collaborator

mjwen commented Apr 4, 2023

Also, we need to update the Weight class such that the input shape of forces_weight and stress_weight do not need to be a float. For example, the force weight need to accommodate for three cases

  • a float. It will be multiple by all (N, 3) force components, where N is the number of atoms
  • 1d array with shape (N,). It will be broadcasted into a shape of (N, 3) and then multiple the forces. Physically, this enables using a different weight for each atom
  • 2d array with shape (N,3). This will be directly multiplied by the forces, enabling the use of a different weight for each individual component

Since @ipcamit is adapting for the new workflow, we do not need to modify it now. But once the new stuff is functioning, we need to get back to this.

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

No branches or pull requests

3 participants