search

stfc / janus-core

Tools for machine learnt interatomic potentials
https://stfc.github.io/janus-core/
BSD 3-Clause "New" or "Revised" License
9 stars 7 forks source link

remove nans for real now #183

Closed alinelena closed 2 months ago

alinelena commented 2 months ago

fixes #182

alinelena commented 2 months ago

Seems to be fixed.

Only thing to note is I think stress is still duplicated in the output if the values are valid (e.g. for NaCl.cif) - mace_mp_stress and stress, which I assume is related.

It's a more minor issue, but if we did address it, it would probably be an alternative fix to this.

duplication comes from the the fact I do not invalidate the calculator results... if I do that as default shall vanish. happy to do it.

ElliottKasoar commented 2 months ago

What about setting write_results=False when writing out? (Ideally as a default in the kwargs in case there really is a reason to write it)

Given that we’ve set the calculator and explicitly handle any results from it, I don’t think we should ever be losing any information that shouldn’t be set elsewhere (info or array).

I’d probably prefer to keep the results by default if we can.

alinelena commented 2 months ago

What about setting write_results=False when writing out? (Ideally as a default in the kwargs in case there really is a reason to write it)

Given that we’ve set the calculator and explicitly handle any results from it, I don’t think we should ever be losing any information that shouldn’t be set elsewhere (info or array).

I’d probably prefer to keep the results by default if we can.

i think now is fixed. at least I do not see duplicates by default. we do not loose any information.