Conversation
Member
|
Hi @jap93, thanks for this! Assuming you would like us to start to review this, can you have a go at applying the pre-commit (see here) and ensuring the tests pass? I think the tests are likely failing due to unpinning aiida-core (see #262 and the linked PR). While we certainly do need to unpin this, it requires additional fixes that @oerc0122 will hopefully contribute, so I wouldn't change it as part of this PR. It would also be great if you could please try to revert any changes to unrelated files e.g. the singlepoint notebook tutorial. |
Contributor
|
I think this should still be in "draft" mode? @jap93 if you look around the web interface there should be a "convert to draft" button somewhere on this page so Elliott knows it shouldn't get final review/merge yet. Clearly the priority at the moment is to get the tests working! I noticed in the output which is probably a code error. The JSON stuff seems more aiida internal and might be related to version incompatibilities. It looks like #262 which was "fixed" by pinning aiida-core. |
ajjackson
reviewed
Mar 13, 2026
ajjackson
left a comment
ajjackson
left a comment
Contributor
There was a problem hiding this comment.
I starting making some suggestions, mostly code style stuff. But noticed that some of these things should be caught by the ruff linter, and indeed there is a related CI failure here. https://github.com/stfc/aiida-mlip/actions/runs/23048418654/job/66943239378?pr=266
I think the preferred approach on aiida-mlip is to setup pre-commit hooks, so the code doesn't get committed without passing linting (and some automatic fixes can be applied). Did you run pre-commit install yet? https://github.com/stfc/aiida-mlip?tab=readme-ov-file#development
(If you need to commit in an emergency it is possible to skip with git commit --no-verify. I find that useful for saving work in progress and running for a bus, but would usually amend/rebase to fix the commit later...)
https://graphite.com/guides/git-commit-skip-hooks#how-to-skip-git-commit-hooks
ajjackson
reviewed
Mar 31, 2026
Contributor
|
I've tried rebasing this branch on @jap93 can you please rebase this on |
Author
|
I think it has completed tests
From: Adam J. Jackson ***@***.***>
Sent: 31 March 2026 15:58
To: stfc/aiida-mlip ***@***.***>
Cc: Purton, John (STFC,DL,SC) ***@***.***>; Mention ***@***.***>
Subject: Re: [stfc/aiida-mlip] add phonons calculation entry point (PR #266)
[Image removed by sender.]ajjackson left a comment (stfc/aiida-mlip#266)<#266?email_source=notifications&email_token=BC7JDN3MDZBMUKZ36JWV5EL4TPMI7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJWGMZDQOJWGI2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4163289624>
I've tried rebasing this branch on main and reverting the aiida-core dependency to 2.7.1 (despite the yank...). With RabbitMQ 3.8.14 running this passes tests on my laptop.
@jap93<https://github.com/jap93> can you please rebase this on main and consider that dependency revert? Would be great to get a green pipeline so we can review properly.
—
Reply to this email directly, view it on GitHub<#266?email_source=notifications&email_token=BC7JDN3MDZBMUKZ36JWV5EL4TPMI7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJWGMZDQOJWGI2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4163289624>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BC7JDN3B2VPOTBSIDYZ4MQD4TPMI7AVCNFSM6AAAAACWIMSFL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCNRTGI4DSNRSGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
ajjackson
reviewed
Apr 1, 2026
Contributor
|
@alinelena @ElliottKasoar I have done some preliminary code review and John P has implemented my suggestions. Would this be a good time from someone from aiida-mlip team to review? |
oerc0122
requested changes
Apr 13, 2026
oerc0122
reviewed
Apr 24, 2026
Contributor
|
@jap93 it looks like this PR needs rebasing to resolve conflicts and get the checks running again so we can finish review. |
oerc0122
requested changes
Apr 30, 2026
Contributor
oerc0122
reviewed
May 28, 2026
oerc0122
reviewed
May 28, 2026
oerc0122
reviewed
May 28, 2026
oerc0122
left a comment
oerc0122
left a comment
Collaborator
There was a problem hiding this comment.
Needs a rebase first and a couple of suggestions. This is pretty much there.
Member
There was a problem hiding this comment.
Hi @jap93, thanks for all of this, it's looking great! Most of the comments I've left are very minor things to tidy up. Please do make sure the examples/tutorials all run with the latest changes you make.
One slightly larger question is about the options we don't present. I know there's been discussion about geometry optimisation, which makes sense as we can use the actual calcjob for that, but there are options such as the thermal properties and DOS/PDOS kwargs that are also not exposed.
Has this been discussed? Generally the guidance for plugins is not to limit users more than they are in the underling code.
A simple option would be to have something similar to MD where we permit an extra set of arbitrary kwargs that we then pass on, or is there a reason not to support these?
I suggest you mark comments from myself and others as "resolved" unless there's anything you're not sure about, since we'll need to close all of these before this is merged.
Author
|
Hi Elliot
Thanks for the comments. We have not discussed thermal style calculations as this is not in the scope of our project and we are in a bit of a rush to get a demo out. Having said that, I could envisage calculating thermodynamic data from MLIP’s as useful. It is probably worth discussing with Alin and Adam to see whether they want to go down that route.
John
From: Elliott Kasoar ***@***.***>
Sent: 25 June 2026 15:43
To: stfc/aiida-mlip ***@***.***>
Cc: Purton, John (STFC,DL,SC) ***@***.***>; Mention ***@***.***>
Subject: Re: [stfc/aiida-mlip] add phonons calculation entry point (PR #266)
@ElliottKasoar commented on this pull request.
Hi @jap93<https://github.com/jap93>, thanks for all of this, it's looking great! Most of the comments I've left are very minor things to tidy up.
One slightly larger question is about the options we don't present. I know there's been discussion about geometry optimisation, which makes sense as we can use the actual calcjob for that, but there are options such as the thermal properties and DOS/PDOS kwargs that are also not exposed.
Has this been discussed? Generally the guidance for plugins is not to limit users more than they are in the underling code.
A simple option would be to have something similar to MD where we permit an extra set of arbitrary kwargs that we then pass on, or is there a reason not to support these?
I suggest you mark comments from myself and others as "resolved" unless there's anything you're not sure about, since we'll need to close all of these before this is merged.
________________________________
In aiida_mlip/parsers/ph_parser.py<#266 (comment)>:
+ If the ProcessNode being passed was not produced by a SinglepointCalc.
+ """
+
+ def __init__(self, node: ProcessNode):
+ """
+ Check that the ProcessNode being passed was produced by a `Singlepoint`.
⬇️ Suggested change
- If the ProcessNode being passed was not produced by a SinglepointCalc.
- """
-
- def __init__(self, node: ProcessNode):
- """
- Check that the ProcessNode being passed was produced by a `Singlepoint`.
+ If the ProcessNode being passed was not produced by a PhononCalc.
+ """
+
+ def __init__(self, node: ProcessNode):
+ """
+ Check that the ProcessNode being passed was produced by a `PhononCalc`.
________________________________
In examples/calculations/submit_phonon.py<#266 (comment)>:
+ params["pdos"] = False
+
+ if bands:
+ params["bands"] = True
+ else:
+ params["bands"] = False
+
+ if symmetrize:
+ params["symmetrize"] = True
+ else:
+ params["symmetrize"] = False
+
+ if len(qpoint_file) > 0:
+ params["qpoint_file"] = qpoint_file
+
+ # Submit single point
⬇️ Suggested change
- # Submit single point
+ # Submit phonon calculation
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+
+ assert "results_dict" in result
+ assert "phonon_output" in result
+
+ lattice_vectors = result["results_dict"].get_dict()["primitive_cell"]["lattice"]
+ supercell_matrix = result["results_dict"].get_dict()["supercell_matrix"]
+ fc = result["results_dict"].get_dict()["force_constants"]["elements"]
+
+ assert lattice_vectors[0][1] == pytest.approx(2.815, rel=1.0e-4, abs=1.0e-4)
+ assert (np.diag(lattice_vectors) == 0.0).all()
+ assert (np.diag(supercell_matrix) == 3).all()
+ assert fc[0][0][0] == pytest.approx(2.2240705622516, rel=1.0e-4, abs=1.0e-4)
+
+
+def test_output_files(fixture_sandbox, generate_calc_job, janus_code, model_folder):
+ """Est setting log and summary output files."""
⬇️ Suggested change
- """Est setting log and summary output files."""
+ """Test setting log and summary output files."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+
+ assert "results_dict" in result
+ assert "phonon_output" in result
+
+ lattice_vectors = result["results_dict"].get_dict()["primitive_cell"]["lattice"]
+ supercell_matrix = result["results_dict"].get_dict()["supercell_matrix"]
+ fc = result["results_dict"].get_dict()["force_constants"]["elements"]
+
+ assert lattice_vectors[0][1] == pytest.approx(2.815, rel=1.0e-4, abs=1.0e-4)
+ assert (np.diag(lattice_vectors) == 0.0).all()
+ assert (np.diag(supercell_matrix) == 2).all()
+ assert fc[0][0][0] == pytest.approx(2.1912727829831, rel=1.0e-4, abs=1.0e-4)
+
+
+def test_run_supercell(model_folder, janus_code):
+ """Test running singlepoint calculation."""
⬇️ Suggested change
- """Test running singlepoint calculation."""
+ """Test setting supercell for phonon calculation."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+ model_file = model_folder / "mace_mp_small.model"
+
+ inputs = {
+ "code": janus_code,
+ "metadata": {"options": {"resources": {"num_machines": 1}}},
+ "model": ModelData.from_local(model_file, architecture="mace_mp"),
+ "arch": Str("chgnet"),
+ "struct": StructureData(ase=bulk("NaCl", "rocksalt", 5.63)),
+ }
+
+ with pytest.raises(InputValidationError):
+ generate_calc_job(fixture_sandbox, entry_point_name, inputs)
+
+
+def test_run_ph(model_folder, janus_code):
+ """Test running singlepoint calculation."""
⬇️ Suggested change
- """Test running singlepoint calculation."""
+ """Test running phonon calculation."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+ """Test singlepoint calculation with missing architecture."""
+ entry_point_name = "mlip.ph"
+
+ inputs = {
+ "code": janus_code,
+ "metadata": {"options": {"resources": {"num_machines": 1}}},
+ "config": JanusConfigfile(config_folder / "config_noarch.yml"),
+ "struct": StructureData(ase=bulk("NaCl", "rocksalt", 5.63)),
+ }
+
+ with pytest.raises(InputValidationError):
+ generate_calc_job(fixture_sandbox, entry_point_name, inputs)
+
+
+def test_two_arch(fixture_sandbox, generate_calc_job, model_folder, janus_code):
+ """Test singlepoint calculation with two defined architectures."""
⬇️ Suggested change
- """Test singlepoint calculation with two defined architectures."""
+ """Test invalid phonon calculation with two defined architectures."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+ """Test singlepoint calculation with missing model."""
+ entry_point_name = "mlip.ph"
+
+ inputs = {
+ "code": janus_code,
+ "metadata": {"options": {"resources": {"num_machines": 1}}},
+ "config": JanusConfigfile(config_folder / "config_nomodel.yml"),
+ "struct": StructureData(ase=bulk("NaCl", "rocksalt", 5.63)),
+ }
+
+ with pytest.raises(InputValidationError):
+ generate_calc_job(fixture_sandbox, entry_point_name, inputs)
+
+
+def test_ph_noarch(fixture_sandbox, generate_calc_job, config_folder, janus_code):
+ """Test singlepoint calculation with missing architecture."""
⬇️ Suggested change
- """Test singlepoint calculation with missing architecture."""
+ """Test invalid phonon calculation with missing architecture."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+ """Test singlepoint calculation with error input."""
+ entry_point_name = "mlip.ph"
+ model_file = model_folder / "mace_mp_small.model"
+ inputs = {
+ "metadata": {"options": {"resources": {"num_machines": 1}}},
+ "code": janus_code,
+ "arch": Str("mace"),
+ "model": ModelData.from_local(model_file, architecture="mace"),
+ "device": Str("cpu"),
+ }
+ with pytest.raises(InputValidationError):
+ generate_calc_job(fixture_sandbox, entry_point_name, inputs)
+
+
+def test_ph_nomodel(fixture_sandbox, generate_calc_job, config_folder, janus_code):
+ """Test singlepoint calculation with missing model."""
⬇️ Suggested change
- """Test singlepoint calculation with missing model."""
+ """Test invalid phonon calculation with missing model."""
________________________________
In tests/calculations/test_phonon.py<#266 (comment)>:
+ ]
+
+ # Check the attributes of the returned `CalcInfo`
+ assert sorted(fixture_sandbox.get_content_list()) == sorted(
+ ["aiida.xyz", "mlff.model"]
+ )
+
+ assert isinstance(calc_info, datastructures.CalcInfo)
+ assert isinstance(calc_info.codes_info[0], datastructures.CodeInfo)
+ calc_str = map(str, calc_info.codes_info[0].cmdline_params)
+ assert sorted(calc_str) == sorted(cmdline_params)
+ assert sorted(calc_info.retrieve_list) == sorted(retrieve_list)
+
+
+def test_ph_nostruct(fixture_sandbox, generate_calc_job, model_folder, janus_code):
+ """Test singlepoint calculation with error input."""
⬇️ Suggested change
- """Test singlepoint calculation with error input."""
+ """Test invalid phonon calculation with missing structure."""
________________________________
In examples/tutorials/calculations/singlepoint.ipynb<#266 (comment)>:
- "# model = ModelData.from_local(\"/path/to/model\", architecture=\"mace\")"
+ "from aiida_mlip.data.model import ModelData\n",
+ "model = ModelData.from_local(\"/mnt/87bd3caf-192d-48ed-9f63-c5afcf32b9ec/ML/janus_work/models/MACE-matpes-r2scan-omat-ft.model\", architecture=\"mace\")"
I don't think we want this change (or necessarily any changes to singlepoint.ipynb, unless there's a strong reason)?
model was commented out because we've already downloaded one above, while this is a path that's specific to the user, who won't necessarily have /mnt/...
________________________________
In examples/tutorials/calculations/phonon.ipynb<#266 (comment)>:
+ "#model = ModelData.from_uri(uri, architecture=\"mace_mp\", cache_dir=\"mlips\")"
+ ]
+ },
+ {
+ "cell_type": "markdown",
+ "metadata": {
+ "id": "Cf28b-C1uM_I"
+ },
+ "source": [
+ "If we already have the model saved in some folder we can save it as:"
+ ]
+ },
+ {
+ "cell_type": "code",
+ "execution_count": null,
+ "metadata": {
+ "id": "sqoVIB2guSKM"
+ },
+ "outputs": [],
+ "source": [
+ "model = ModelData.from_local(\"/path/to/model\", architecture=\"mace\")"
I think we normally have these commented out the other way? For most users, it's easiest to use the from_uri option, and the from_local is more for their information later on.
This is also relevant for #192<#192>, since it we want these to be able to run automatically, we need to download the model (or use a real path that would work with the Action, if we really wanted to use from_local).
________________________________
In examples/tutorials/calculations/phonon.ipynb<#266 (comment)>:
+ "metadata": {
+ "id": "mH5E3MtPtyj-"
+ },
+ "outputs": [],
+ "source": [
+ "from aiida.orm import load_code\n",
+ "code = ***@***.***\")"
+ ]
+ },
+ {
+ "cell_type": "markdown",
+ "metadata": {
+ "id": "u-nJa_viuZMv"
+ },
+ "source": [
+ "The other inputs can be set up as AiiDA Str. There is a default for every input except the structure and code. This is a list of possible inputs:"
I know we don't currently do this in other tutorial examples, but given this is the longest list of options yet, perhaps it's worth linking to the janus-core docs or elsewhere to clarify what some of these do?
________________________________
In examples/tutorials/calculations/phonon.ipynb<#266 (comment)>:
+ "inputs = {\n",
+ " \"code\": code,\n",
+ " \"model\": model,\n",
+ " \"struct\": structure,\n",
+ " \"arch\": Str(model.architecture),\n",
+ " \"device\": Str(\"cpu\"),\n",
+ " \"supercell\": Str(\"2 2 2\"),\n",
+ " \"minimize\": Bool(False),\n",
+ " \"displacement\": 0.01,\n",
+ " \"fmax\" : 0.01,\n",
+ " \"nqpoints\": 51,\n",
+ " \"dos\": Bool(False),\n",
+ " \"pdos\": Bool(False),\n",
+ " \"bands\": Bool(False),\n",
+ " \"no_hdf5\": Bool(True),\n",
+ " \"symmetrize\": Bool(False),\n",
+ " \"calc_kwargs\": Dict({\"dispersion\": False}),\n",
+ " \"metadata\": {\"options\": {\"resources\": {\"num_machines\": 1}}},\n",
+ " }"
Are all of these inputs actually defined? I get
ValueError: Error occurred validating port 'inputs': Unexpected ports {'minimize': <Bool: uuid: 5c2e7b90-e379-41c4-8f2f-0c5589302391 (unstored) value: False>, 'fmax': 0.01}, for a non dynamic namespace
I vaguely remember a discussion about removing the geomopt options, so I suspect they were removed from the input spec?
________________________________
In examples/tutorials/calculations/phonon.ipynb<#266 (comment)>:
+ ]
+ },
+ {
+ "cell_type": "code",
+ "execution_count": null,
+ "metadata": {},
+ "outputs": [],
+ "source": [
+ "! verdi process list -a"
+ ]
+ },
+ {
+ "cell_type": "markdown",
+ "metadata": {},
+ "source": [
+ "And see the results we are interested in. Substitute the number with the PK number of your calculation"
I don't think it's worth blocking merging this, but it would probably be useful to show how to get and plot at least one of the outputs (bands/DOS/PDOS), otherwise this all feels quite abstract
________________________________
In aiida_mlip/calculations/phonon.py<#266 (comment)>:
+ spec.input(
+ "properties",
+ valid_type=Str,
+ required=False,
+ help="Properties to calculate",
+ )
This isn't a janus phonons input?
________________________________
In aiida_mlip/calculations/phonon.py<#266 (comment)>:
+
+ spec.input(
+ "properties",
+ valid_type=Str,
+ required=False,
+ help="Properties to calculate",
+ )
+
+ spec.input(
+ "supercell",
+ valid_type=Str,
+ required=False,
+ help="The size of supercells used in phonon calculation",
+ )
+ spec.input(
+ "nqpoints",
We might want to consider n_qpoints (-> --n-qpoints) to be consistent with janus-core's input?
________________________________
In aiida_mlip/calculations/phonon.py<#266 (comment)>:
+ help="The size of supercells used in phonon calculation",
+ )
+ spec.input(
+ "nqpoints",
+ valid_type=Int,
+ required=False,
+ help="Number of q points in band path",
+ )
+ spec.input(
+ "displacement",
+ valid_type=Float,
+ required=False,
+ help="Displacement for numerical derivatives",
+ )
+ spec.input(
+ "no_hdf5",
Having the default be a double negative is slightly confusing? Was there a reason for this choice?
________________________________
In aiida_mlip/calculations/phonon.py<#266 (comment)>:
+ ]
+ codeinfo.cmdline_params += ["--n-qpoints", nqpoints]
+
+ # properties left in just in case for further expansion
+ if "properties" in self.inputs:
+ properties = self.inputs.properties.value
+ codeinfo.cmdline_params += ["--properties", properties]
+
+ calcinfo.retrieve_list.extend(
+ [
+ phonon_filename,
+ "aiida-force_constants.hdf5",
+ "aiida-dos.dat",
+ "aiida-pdos.dat",
+ "aiida-auto_bands.yml.xz",
+ # "aiida-bands.hdf5",
Why is this commented out?
________________________________
In aiida_mlip/calculations/phonon.py<#266 (comment)>:
+ # properties left in just in case for further expansion
+ if "properties" in self.inputs:
+ properties = self.inputs.properties.value
+ codeinfo.cmdline_params += ["--properties", properties]
Expansion of what? Currently setting this throws an error
No such option: --properties ('(Possible options: --progress-bar)',) │
________________________________
In docs/source/user_guide/calculations.rst<#266 (comment)>:
+
+Phonon calculations are executed in a similar manner to the previous styles of calculation using either the ``run`` or ``submit`` Aiida commands.
+An example of how to execute a calculation is shown below:
+
+.. code-block:: python
+
+ inputs = {
+ "metadata": metadata,
+ "code": code,
+ "arch": Str,
+ "struct": structure,
+ "model": model,
+ "device": Str,
+ #optional inputs below
+ "supercell": Str
+ "minimize" : Bool,
⬇️ Suggested change
- "minimize" : Bool,
Not an option?
________________________________
In docs/source/user_guide/calculations.rst<#266 (comment)>:
+Phonon calculations are executed in a similar manner to the previous styles of calculation using either the ``run`` or ``submit`` Aiida commands.
+An example of how to execute a calculation is shown below:
+
+.. code-block:: python
+
+ inputs = {
+ "metadata": metadata,
+ "code": code,
+ "arch": Str,
+ "struct": structure,
+ "model": model,
+ "device": Str,
+ #optional inputs below
+ "supercell": Str
+ "minimize" : Bool,
+ "nohdf5" : Bool,
⬇️ Suggested change
- "nohdf5" : Bool,
+ "no_hdf5" : Bool,
Currently, I think?
________________________________
In docs/source/user_guide/calculations.rst<#266 (comment)>:
+ verdi run submit_phonon.py ***@***.***" --structure "path/to/structure" --model "path/to/model"
+ #with some optional flags
+ verdi run submit_phonon.py ***@***.***" --structure "path/to/structure" --model "path/to/model" --supercell "2 2 2" --dos --bands
⬇️ Suggested change
- verdi run submit_phonon.py ***@***.***" --structure "path/to/structure" --model "path/to/model"
- #with some optional flags
- verdi run submit_phonon.py ***@***.***" --structure "path/to/structure" --model "path/to/model" --supercell "2 2 2" --dos --bands
+ verdi run submit_phonon.py ***@***.***" --struct "path/to/structure" --model "path/to/model"
+ #with some optional flags
+ verdi run submit_phonon.py ***@***.***" --struct "path/to/structure" --model "path/to/model" --supercell "2 2 2" --dos --bands
________________________________
In examples/calculations/submit_phonon.py<#266 (comment)>:
+ "--arch",
+ default="mace",
+ type=str,
+ help="MLIP architecture to use for calculations.",
+)
***@***.***(
+ "--device", default="cpu", type=str, help="Device to run calculations on."
+)
***@***.***(
+ "--supercell",
+ default="2 2 2",
+ type=str,
+ help="The size of supercell matrix to calculate phonons e.g. 2 2 2.",
+)
***@***.***(
+ "--nohdf5",
I would use --no-hdf5 (or --hdf5) to be consistent with janus-core
________________________________
In examples/tutorials/workgraphs/geom_phonon.ipynb<#266 (comment)>:
+ "model = ModelData.from_local(\"/path/to/model\", architecture=\"mace\")\n",
+ "\n",
+ "#or from url\n",
+ "#uri = \"https://github.com/stfc/janus-core/raw/main/tests/models/mace_mp_small.model\"\n",
+ "#model = ModelData.from_uri(uri, architecture=\"mace\", cache_dir=\"mlips\")"
As elsewhere, I think the other way around makes more sense
________________________________
In examples/tutorials/workgraphs/geom_phonon.ipynb<#266 (comment)>:
+ " \"minimize\": Bool(False),\n",
+ " \"fmax\": Float(0.1),\n",
⬇️ Suggested change
- " \"minimize\": Bool(False),\n",
- " \"fmax\": Float(0.1),\n",
No longer options?
________________________________
In examples/tutorials/workgraphs/geom_phonon.ipynb<#266 (comment)>:
+ ]
+ },
+ {
+ "cell_type": "markdown",
+ "id": "4a19aaa5",
+ "metadata": {},
+ "source": [
+ "#### Phonon Calculation"
+ ]
+ },
+ {
+ "cell_type": "markdown",
+ "id": "7609565d",
+ "metadata": {},
+ "source": [
+ "The results for the phonon calculation were not passed to the graph so the output can be retrieved using the alternate method. Note the results use the optimised lattice parameter for the phonon calculation."
As previously, ideally we'd maybe see something more visual, but that shouldn't block merging
—
Reply to this email directly, view it on GitHub<#266?email_source=notifications&email_token=BC7JDN3JGGULSQGXHSUYOA35BU27PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGE2TIMZXGYY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4571543761>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BC7JDNZ7P7N2MGLLSDDXFX35BU27PAVCNFSNUABFKJSXA33TNF2G64TZHM3TKMBYGM2DAMBSHNEXG43VMU5TIMBSHA4TGOBZG42KC5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/BC7JDN6PCDNFWYULKT245QL5BU27PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGE2TIMZXGYY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/BC7JDNZTPQSHOYLD5MRTSCD5BU27PA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGE2TIMZXGYY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Contributor
|
Right, the priority to support other projects is to get force constants and then do property calculations with Euphonic rather than do everything with Phonopy in one go. That said, it is a little bit odd to produce bands and not DOS, because for systems with many atoms in primitive cell the DOS is the more intelligible representation of the data. The thermochemistry outputs are used much less and I don't expect they'll be missed for a while. |
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Co-authored-by: Elliott Kasoar <45317199+ElliottKasoar@users.noreply.github.com>
Member
|
Hi @jap93, thanks for all of the fixes. A few of the threads marked as resolved don't seem to have included any changes. I haven't gone through all of them, but, for example, the input spec still seems to include Did you push all of your changes? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a pull request to include a phonon component into aiida-mlip. At present a phonopy.yaml file is passed back from janus-core to aiida-mlip. Further work will be to use hdf5 format to return force constants and include band structure.