search

ACEsuit / mace

MACE - Fast and accurate machine learning interatomic potentials with higher order equivariant message passing.
Other
413 stars 157 forks source link

Error reading 'extended' xyz format files #415

Open venkatkapil24 opened 1 month ago

venkatkapil24 commented 1 month ago

Describe the bug MACE returns error for an ".extxyz" file.

Traceback (most recent call last): File "/home/ucapvka/source/mace/scripts/run_train.py", line 6, in main() File "/lustre/home/ucapvka/mace-venv/lib/python3.9/site-packages/mace/cli/run_train.py", line 157, in main assert args.train_file.endswith(".xyz"), "Must specify atomic_numbers when using .h5 train_file input" AssertionError: Must specify atomic_numbers when using .h5 train_file input

Typically XYZ files do not include anything other than positions/velovities. On the other hand, the extended XYZ in ASE/libatoms '.extxyz' is the standard format. I think MACE should include it and not assume .h5. Again, happy to open a quick PR.

ilyes319 commented 1 month ago

Hey @venkatkapil24, MACE can read extended xyz, but you need to rename the extension just .xyz.

bernstei commented 1 month ago

That's essentially a bug, I'd argue. If anything, I'd say it should required ".extxyz"

gabor1 commented 1 month ago

I agree it should accept both .xyz and .extxyz

bernstei commented 1 month ago

I'm not even convinced it should check anything except, I guess, special-casing .h5. Anything else it could just pass to ase.io.read and let it deal with it automatically.

venkatkapil24 commented 1 month ago

I'm not even convinced it should check anything except, I guess, special-casing .h5. Anything else it could just pass to ase.io.read and let it deal with it automatically.

I agree. This will also help read directly from json.

bernstei commented 1 month ago

I agree. This will also help read directly from json.

iff it's json that ase.io.read can parse

stargolike commented 3 weeks ago

today i upgrade my mace, and i meet this problem. it's not good idea, yesterday i can use extxyz and now i can't.

ilyes319 commented 3 weeks ago

just change your extension to .xyz for now.

gabor1 commented 3 weeks ago

ASE changes its behaviour randomly... we are aware of it.

bernstei commented 3 weeks ago

yesterday i can use extxyz and now i can't.

I'm actually very surprised that using extxyz isn't working. If anything, I'd expect ASE to have decided that plain xyz is not for extxyz.

@stargolike can you say precisely what versions of mace and ase you're trying to use? I just confirmed that the latest ase (3.23.0) reads .xyz and .extxyz suffix files with no problem.

bernstei commented 3 weeks ago

just change your extension to .xyz for now.

@ilyes319 Did you actually change something in MACE to cause this change?

stargolike commented 3 weeks ago

ASE changes its behaviour randomly... we are aware of it.

thanks, it's good method.

stargolike commented 3 weeks ago

ASE changes its behaviour randomly... we are aware of it.

it sounds so sad :(

stargolike commented 3 weeks ago

just change your extension to .xyz for now.

i solve the problem, thank you.

stargolike commented 3 weeks ago

just change your extension to .xyz for now.

@ilyes319 Did you actually change something in MACE to cause this change?

I newly reinstalled MACE, so I met this problem

bernstei commented 3 weeks ago

@ilyes319 Did you actually change something in MACE to cause this change?

I newly reinstalled MACE, so I met this problem

I was asking @ilyes319 if he intentionally changed anything in MACE to cause this. Because there's really no reason it should start happening, unless he hard-wired something, which isn't a great idea.

bernstei commented 3 weeks ago

@ilyes319 Did you actually change something in MACE to cause this change?

I newly reinstalled MACE, so I met this problem

I was asking @ilyes319 if he intentionally changed anything in MACE to cause this. Because there's really no reason it should start happening, unless he hard-wired something, which isn't a great idea.

Never mind - I see in the original post that started this thread that .xyz is hardwired. I stand by my statement that it should just defer to what ASE can read. A PR would be essentially trivial, but I don't know what branch to base it on. @ilyes319 ?

stargolike commented 3 weeks ago

@ilyes319 Did you actually change something in MACE to cause this change?

I newly reinstalled MACE, so I met this problem

I was asking @ilyes319 if he intentionally changed anything in MACE to cause this. Because there's really no reason it should start happening, unless he hard-wired something, which isn't a great idea.

i use

pip3 install torch torchvision torchaudio --extra-index-url https://download.pytorch.org/whl/cu116
git clone https://github.com/ACEsuit/mace.git
pip install ./mace

today i reinstall MACE in a new machine, and i think when i'm running this command, the lastest version ASE was installed.

gabor1 commented 3 weeks ago

We could temporarily pin ASE 3.22 like everyone else...

bernstei commented 3 weeks ago

OMFG. No. This has nothing to do with ASE's version. ".xyz" is hard-coded into MACE, for no good reason.

bernstei commented 3 weeks ago

OMFG. No. This has nothing to do with ASE's version. ".xyz" is hard-coded into MACE, for no good reason.

It needs to distinguish .h5 from things that are read with ase.io.read, but that's the only issue. It's easy to clean up, as soon as I know what branch to do it relative to.

venkatkapil24 commented 3 weeks ago

Yes, this is a mace issue. I can only read .xyz or .h5.

bernstei commented 3 weeks ago

PR coming soon.