Add ElectronicDOS and XASSpectra properties #54

JosePizarro3 · 2024-04-12T11:23:35Z

@JFRudzinski @Bernadette-Mohr only for you: I added properties/energies.py and a couple of properties, FermiLevel and ChemicalPotential. You can disregard the rest of the message.

I just finished this initial version. Mainly, you have to check properties/spectral_profile.py and properties/energies.py. I added some tests, but I have the issue that I would need some other sections in order to properly test the functionalities in ElectronicDensityOfStates, namely, I need ElectronicEigenvalues containing the info of the highest_occupied_energy and lowest_occupied_energy.

Besides these, I changed OrbitalsState and AtomsState to inherit from Entity. Do you mind giving your thoughts in here? I wanted to give an entity_ref that can be used to reference if a property is atomic or orbitally resolved, e.g., the projected DOS (but there might be others).

coveralls · 2024-04-16T07:56:19Z

Pull Request Test Coverage Report for Build 8875188200

Details

196 of 200 (98.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.4%) to 97.101%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/conftest.py	54	58	93.1%

Totals
Change from base Build 8753733270:	1.4%
Covered Lines:	335
Relevant Lines:	345

💛 - Coveralls

Added iri to quantity in PhysicalProperty

Fix __setattr__ in PhysicalProperty

Added Nathan comments

Added raise ValueError in PhysicalProperty when value is None

Added SpectralProfile, ElectronicSpectralProfile Added basic ElectronicDensityOfStates

Added ElectronicDOS and XASSpectra

Added SpectralProfile, ElectronicDensityOfStates, XASSpectra in spectral_profile.py Check iri assignement in __init__ in PhysicalProperty Fixed ElectronicBandGap

…fStates Added initial test modules

JFRudzinski · 2024-04-19T13:01:09Z

regard the rest of the messa

Thanks @JosePizarro3, I did see this. I will eventually use this as a basis and combine with the other energies that I had written down

ndaelman-hu

Here's a 1st set of feedback. Revising the tests comes later.

src/nomad_simulations/atoms_state.py

src/nomad_simulations/properties/energies.py

src/nomad_simulations/properties/__init__.py

src/nomad_simulations/properties/spectral_profile.py

ndaelman-hu · 2024-04-22T11:12:37Z

src/nomad_simulations/properties/spectral_profile.py

+        # TODO (cont'd) and `entity_ref` is set)
+
+
+class ElectronicDensityOfStates(DOSProfile):


Given the complexity of the normalizers in this class, I think it is best to develop an electronic structure post-analysis suite, similar to MatID for symmetries. There already exist PyProcar and ASH, but they're fairly limited and code specific.

@JFRudzinski where would this fit into our planning? Note that a proper package would be worthy of a publication!

I think it is best to develop an electronic structure post-analysis suite, similar to MatID for symmetries.

Yeah it might be, but we need the whole picture first. IMO, we could publish this repo as it is, and later think of some post-processing analysis tools which could be generalized. As of now, it is true that I am trying to keep as much as possible from the former normalization, but improving the schema.

Okay, the standalone module is more an idea for down the line (@JFRudzinski would also like your feedback) and a way to get an easy paper. I'm not suggesting this for now! Still, it feels a bit out of place compared to the simpler, and more to-the-point checks that make up most of the other normalizers.

What is relevant at the moment, though, is stress testing these algos. We have numerous cases in our repo, where the band structure or DOS extraction fail, and it's painful to see. This has always been a very tricky issue.
Since this PR focuses on migrating and extending the schema, I implore you to open a new issue where we tackle this aspect. In the meantime, I'll think up more test cases.

src/nomad_simulations/properties/spectral_profile.py

Add TODOs

JosePizarro3 · 2024-04-23T09:38:34Z

@ndaelman-hu thanks for the comments

Would you mind checking again? I can also discuss the details on implementation is something is unclear.

ndaelman-hu · 2024-04-23T11:38:17Z

@ndaelman-hu thanks for the comments

Would you mind checking again? I can also discuss the details on implementation is something is unclear.

Okay, will take a look somewhere this evening.

src/nomad_simulations/properties/band_gap.py

ndaelman-hu

Almost there. Mostly small stuff, and a bit of reshuffling of where code belongs.

I still need to take a look at larger tests in test_spectral_profile, but I'll do that after a break.

tests/conftest.py

tests/test_energies.py

tests/conftest.py

tests/test_spectral_profile.py

src/nomad_simulations/properties/spectral_profile.py

ndaelman-hu · 2024-04-24T09:42:43Z

src/nomad_simulations/properties/spectral_profile.py

+        # TODO (cont'd) and `entity_ref` is set)
+
+
+class ElectronicDensityOfStates(DOSProfile):


Okay, the standalone module is more an idea for down the line (@JFRudzinski would also like your feedback) and a way to get an easy paper. I'm not suggesting this for now! Still, it feels a bit out of place compared to the simpler, and more to-the-point checks that make up most of the other normalizers.

What is relevant at the moment, though, is stress testing these algos. We have numerous cases in our repo, where the band structure or DOS extraction fail, and it's painful to see. This has always been a very tricky issue.
Since this PR focuses on migrating and extending the schema, I implore you to open a new issue where we tackle this aspect. In the meantime, I'll think up more test cases.

src/nomad_simulations/properties/spectral_profile.py

ndaelman-hu

So, I brought up some additional edge cases to test for.
The failing tests are what worry me most, though. Since it is all related to the pDOS, it could be tackled in a separate issue, but that has to be high on the priority list.

Regarding DOS in general, it really doesn't merit a fundamentally different treatment from other spectra. I think we can have a single class where name is used to specify the profile type.

tests/test_spectral_profile.py

src/nomad_simulations/properties/spectral_profile.py

tests/test_spectral_profile.py

JosePizarro3 · 2024-04-25T11:37:52Z

So, I brought up some additional edge cases to test for. The failing tests are what worry me most, though. Since it is all related to the pDOS, it could be tackled in a separate issue, but that has to be high on the priority list.

Regarding DOS in general, it really doesn't merit a fundamentally different treatment from other spectra. I think we can have a single class where name is used to specify the profile type.

Ok, tricky error in the conftest for fixing the testing for pDOS: I need to create a full Simulation object, and not simply let ModelSystem hang around. I will push something today so you can check the new generate_simulation_electronic_dos conftests (which returns the full Simulation, and not the ElectronicDensityOfStates section).

JosePizarro3 · 2024-04-25T14:44:41Z

Ok, finished. @ndaelman-hu do you mind checking before going on holidays? Thanks!

ndaelman-hu

All good, thank you very much!!

tests/test_spectral_profile.py

ndaelman-hu · 2024-04-26T14:17:52Z

src/nomad_simulations/properties/spectral_profile.py

+            self.m_cache['lowest_occupied_energy'] = lowest_occupied_energy
+
+        # Set thresholds for the energies and values
+        energy_threshold = config.normalize.band_structure_energy_tolerance


My IDE is telling me that config has no member normalize. Are you using a different branch then develop for your base nomad installation?

I am using a branch where I added this repo as a submodule. I saw the config and models/config + check locally and I can use the normalize no problem.

tests/test_spectral_profile.py

tests/conftest.py

JosePizarro3 · 2024-04-29T07:59:25Z

Thanks @ndaelman-hu !

I am merging this and continue working on #52

JosePizarro3 self-assigned this Apr 12, 2024

JosePizarro3 changed the title ~~Add DOS output~~ Add ElectronicDOS and XASSpectra properties Apr 15, 2024

JosePizarro3 changed the title ~~Add ElectronicDOS and XASSpectra properties~~ Add ElectronicDOS and XASSpectra properties Apr 15, 2024

JosePizarro3 force-pushed the 32-add-dos-output branch 2 times, most recently from b7f7d57 to 33a1871 Compare April 16, 2024 07:52

JosePizarro3 force-pushed the 32-add-dos-output branch 2 times, most recently from 163e0cc to f8a8426 Compare April 18, 2024 07:41

Base automatically changed from 31-add-bandgap-output to develop April 18, 2024 08:46

JosePizarro3 force-pushed the 32-add-dos-output branch from f8a8426 to 6eb1b5f Compare April 18, 2024 08:56

JosePizarro3 linked an issue Apr 18, 2024 that may be closed by this pull request

Add ElectronicDOS and XASSpectra properties #32

Closed

JosePizarro3 force-pushed the 32-add-dos-output branch from e265c43 to 9c3c6ed Compare April 18, 2024 11:46

JosePizarro3 added 18 commits April 19, 2024 14:43

Improved ElectronicBandGap property

e8727fd

Added iri to quantity in PhysicalProperty

Added testing for ElectronicBandGap

253a73d

Fix __setattr__ in PhysicalProperty

Fixed IRI

4a48f4e

Added Nathan comments

Fix negative value to None

9ad814e

Changed negative value of ElectronicBandGap to be set to None

4b1e922

Added raise ValueError in PhysicalProperty when value is None

Fix logic for momentum transfer and resolve_type

044085c

Added PhysicalProperty in outputs.py

9362e4e

Added SpectralProfile, ElectronicSpectralProfile Added basic ElectronicDensityOfStates

Improved schema for spectral profile

b5b6636

Added ElectronicDOS and XASSpectra

Added utils in outputs.py

79e4dc8

Added more functionalities in spectral_profile.py

3212713

Added energies_origin

29c9e18

Fix rebasing to PhysicalProperty

ce1c4b4

Moved define properties to subfolder

9a31d01

Added energies.py and FermiLevel and ChemicalPotential

e6633fd

Added SpectralProfile, ElectronicDensityOfStates, XASSpectra in spectral_profile.py Check iri assignement in __init__ in PhysicalProperty Fixed ElectronicBandGap

Fix after rebasing

ea2e717

Fix comments

2596fa0

Fix mypy

55e9f41

Added better functionalities from DOSNormalizer to ElectronicDensityO…

76203d9

…fStates Added initial test modules

Fix mypy

df913b0

JosePizarro3 force-pushed the 32-add-dos-output branch from 1b00e42 to df913b0 Compare April 19, 2024 12:45

JosePizarro3 requested a review from ndaelman-hu April 19, 2024 12:53

ndaelman-hu reviewed Apr 22, 2024

View reviewed changes

JosePizarro3 added 3 commits April 23, 2024 09:40

Fix typos

1d9d5ea

Deleted fermi_level from ElectronicDOS and method resolve_fermi_level

ff6c307

Add TODOs

Delete custom_physical_property from Outputs

6100923

JosePizarro3 requested a review from ndaelman-hu April 23, 2024 09:38

ndaelman-hu reviewed Apr 24, 2024

View reviewed changes

src/nomad_simulations/properties/band_gap.py Show resolved Hide resolved

ndaelman-hu approved these changes Apr 24, 2024

View reviewed changes

ndaelman-hu self-requested a review April 24, 2024 19:19

ndaelman-hu requested changes Apr 24, 2024

View reviewed changes

Added feedback by Nathan

bd024c1

JosePizarro3 added 5 commits April 25, 2024 13:41

Fix testing for pDOS

05a620a

Merged remote branch and solved conflict

60d273b

Added resolve_pdos_name in DOSProfile and fixed conftest

ef2ae48

Added test_resolve_pdos_name

7600bd8

Improved generate_from_contributions in XASSpectra

fa6e576

JosePizarro3 requested a review from ndaelman-hu April 25, 2024 14:44

Add todo note for value units and normalization_factor

6c614d5

ndaelman-hu approved these changes Apr 26, 2024

View reviewed changes

Improved testing

d65cc14

JosePizarro3 merged commit b159b0c into develop Apr 29, 2024
4 checks passed

JosePizarro3 deleted the 32-add-dos-output branch April 29, 2024 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ElectronicDOS and XASSpectra properties #54

Add ElectronicDOS and XASSpectra properties #54

JosePizarro3 commented Apr 12, 2024 •

edited

Loading

coveralls commented Apr 16, 2024 •

edited

Loading

JFRudzinski commented Apr 19, 2024

ndaelman-hu left a comment

ndaelman-hu Apr 22, 2024

JosePizarro3 Apr 22, 2024

ndaelman-hu Apr 24, 2024

JosePizarro3 commented Apr 23, 2024

ndaelman-hu commented Apr 23, 2024

ndaelman-hu left a comment

ndaelman-hu Apr 24, 2024

ndaelman-hu left a comment

JosePizarro3 commented Apr 25, 2024

JosePizarro3 commented Apr 25, 2024

ndaelman-hu left a comment

ndaelman-hu Apr 26, 2024

JosePizarro3 Apr 29, 2024

JosePizarro3 commented Apr 29, 2024

		# TODO (cont'd) and `entity_ref` is set)


		class ElectronicDensityOfStates(DOSProfile):

Add ElectronicDOS and XASSpectra properties #54

Add ElectronicDOS and XASSpectra properties #54

Conversation

JosePizarro3 commented Apr 12, 2024 • edited Loading

coveralls commented Apr 16, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8875188200

Details

💛 - Coveralls

JFRudzinski commented Apr 19, 2024

ndaelman-hu left a comment

Choose a reason for hiding this comment

ndaelman-hu Apr 22, 2024

Choose a reason for hiding this comment

JosePizarro3 Apr 22, 2024

Choose a reason for hiding this comment

ndaelman-hu Apr 24, 2024

Choose a reason for hiding this comment

JosePizarro3 commented Apr 23, 2024

ndaelman-hu commented Apr 23, 2024

ndaelman-hu left a comment

Choose a reason for hiding this comment

ndaelman-hu Apr 24, 2024

Choose a reason for hiding this comment

ndaelman-hu left a comment

Choose a reason for hiding this comment

JosePizarro3 commented Apr 25, 2024

JosePizarro3 commented Apr 25, 2024

ndaelman-hu left a comment

Choose a reason for hiding this comment

ndaelman-hu Apr 26, 2024

Choose a reason for hiding this comment

JosePizarro3 Apr 29, 2024

Choose a reason for hiding this comment

JosePizarro3 commented Apr 29, 2024

JosePizarro3 commented Apr 12, 2024 •

edited

Loading

coveralls commented Apr 16, 2024 •

edited

Loading