search

TREX-CoE / trexio_tools

Set of tools for trexio files
BSD 3-Clause "New" or "Revised" License
18 stars 8 forks source link

changes to read cell matrix pysc2trexio periodic systems #19

Closed josuelandinez closed 1 year ago

josuelandinez commented 1 year ago

Hi,

I found a problem long time ago when using the converter from pyscf to trexio for periodic systems. I just forget to do the request always. It happens when reading the cell matrix sometimes seems to be just a string and it is not converted properly to a numpy array. The error message is like this:

error:" TypeError: ufunc 'divide' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' "

I solved in a simple way with the modifications on this fork. I don't think it generates any conflict or other problem. I would like to ask to @kousuke-nakano or @q-posev to review it before merged if you regard it appropriate.

best,

Edgar

q-posev commented 1 year ago

Hi @josuelandinez,

Thanks! I see that CI is failing with TypeError, so I think that your changes break the compatiibillity. We may need to treat 2 cases separately. It would be good to now when and how it happens that cell matrix is string and not a list/ndarray. Perhaps @kousuke-nakano would have a better idea.

kousuke-nakano commented 1 year ago

Hi @josuelandinez,

Thanks!! Would you give me a PySCF checkpoint file to reproduce the bug you reported?

josuelandinez commented 1 year ago

Hi @q-posev and @kousuke-nakano ,

@q-posev you were right, it is my bad habit to parse the cell with strings as Pyscf allows. Then the changes were breaking compatibility. I just change it to treat the different cases as you suggested and seems to pass all the tests now. I also test my self by passing the cell as a list, np.array and a string as I was doing and now the conversion is done in any of these cases, checkpoint files attached.

I think, it can be merged now, I will be attentive to your answer.

best,

Edgar checkpoints.zip

q-posev commented 1 year ago

Thanks @josuelandinez ! Good that you catched this corner case. I'll wait for @kousuke-nakano feedback but the PR looks good to me.

kousuke-nakano commented 1 year ago

Dear @josuelandinez and @q-posev Thanks!! I totally agree to this change :-)