Extend yambo spectra#158
Conversation
Refactor get_spectra method to parse data for EM and update spectra type based on sp_type.
Removed unnecessary initialization of AbsorptionSpectrum.
| data = [] | ||
| names = [] | ||
|
|
||
| with open(self.filepath) as f: |
There was a problem hiding this comment.
we should introduce a new file parser class for the spectra file, we do not want to append the data to the mainfile parser.
There was a problem hiding this comment.
Sorry, maybe I'm missing something.... By "introducing a new file parser class for the spectra file" do you mean adding something to the definition of the class SpectraParser in file_parsers.py? Apart from that, in parser.py we indeed have a class YamboSpectraParser (into which my colleague Hajar and I have added some lines..). Moreover, are we really appending spectra data to the mainfile parser?
There was a problem hiding this comment.
Yes indeed this is what I meant. We should have a file parser and a mapping parser. What you are trying to do at the moment is parsing the spectra file and appending it to a function call within the mainfile parser. Although this could work, I would like to as much as possible separate the auxiliary files.
There was a problem hiding this comment.
Ok, so, if I understand correctly, what you suggest may mean handling spectra files similarly to how netcdf files are already handled, i.e. we should 1) "populate" the SpectraParser class definition in file_parser.py, possibly putting there some parts that currently are in the YamboSpectraParser class in parser.py, 2) making the YamboSpectraParser class (in parser.py) of MappingParser type, instead of TestParser type.
There was a problem hiding this comment.
Yes indeed. You should put the part that search for spectra files in YamboArchiveWriter. You then create a YamboSpectraParser (mapping parser) in the archive writer similar to the net cdf case. You should add the corresponding annotations in the schema file and use a different annotation key e.g. yambo_spectra
| # add to simulation outputs.absorption_spectra | ||
|
|
||
| spectra_obj = absorption_spectra_parser.data_object | ||
| spectra_obj.label = sp_type |
There was a problem hiding this comment.
we should not do this here but instead add this in the annotation
| else data.m_create(yambo.outputs.Outputs) | ||
| data.outputs[-1] if data.outputs else data.m_create(yambo.outputs.Outputs) | ||
| ) | ||
| outputs.m_append( |
There was a problem hiding this comment.
we should also add this to the Outputs annotation inorder to automate this
| absorption_spectra_parser.annotation_key = yambo.SPECTRA_KEY | ||
|
|
||
| #start HB | ||
| SPECTRA_TYPE_MAP = { |
|
Can you please add some test data with the spectra files? |
| def get_spectra(self, data: np.ndarray) -> dict[str, Any]: | ||
| return dict(excitation_energies=data[:, 0], intensities=data[:, 1]) | ||
| #start HB | ||
| def get_spectra(self) -> dict[str, Any]: |
There was a problem hiding this comment.
this is already covered by the file parser I believe. DataTextParser will simply use np.loadtxt to read the file.
| for spectra_file in spectra_files: | ||
| spectra_parser.filepath = spectra_file | ||
|
|
||
| sp_type = mainfile_parser.data.get('sp_type') |
There was a problem hiding this comment.
this should be spectra_parser.get('sp_type') as you defined it there.
| outputs.AbsorptionSpectrum.sp_type, SPECTRA_KEY, 'sp_type' | ||
| ) | ||
| add_mapping_annotation( | ||
| outputs.AbsorptionSpectrum.n_energies, SPECTRA_KEY, 'n_energies' |
There was a problem hiding this comment.
n_energies is not defined in the spectra parser.
There was a problem hiding this comment.
Yes, indeed. I wish to define it in the SpectraParser class in yambo/file_parsers.py, but in fact I am not sure how to define it, since it is a derived quantity (it is shape[0] of data.outputs, or anyway of the object that contains all the (energy,intensity) pairs of values of a spectrum), not defined by matching a specific regular expression (at a difference with sp_type and many other quantities in that or other file_parsers.py).
There might be an alternative way of defining n_energies, by looking for the line containing the expression "Energy steps" in the o* file(s).
| class Outputs(outputs.Outputs): | ||
| # TODO add description | ||
| sp_type = Quantity(type=str) | ||
| # sp_type = Quantity(type=str) # EM: commented, 6 Jul, 2026 |
There was a problem hiding this comment.
is sp_type already in the nomad-simulations Outputs?
There was a problem hiding this comment.
Indeed it isn't. So, here in /schema_packages/yambo.py, should I uncomment it in the class Outputs or rather put it in the class AbsorptionSpectra?
There was a problem hiding this comment.
Ah i get it now, sorry it is already in AbsorptionSpectra.
There was a problem hiding this comment.
adsorption_spectra is not in nomad-simulation Outputs. so maybe add
adsorption_spectra = SubSection(sub_section=AbsorptionSpectra.m_def, repeats=True)
under class Outputs in yambo.py
There was a problem hiding this comment.
Well, in fact we do have a line:
absorption_spectra = SubSection(sub_section=AbsorptionSpectrum.m_def, repeats=True)
in nomad_simulations/schema_packages/outputs.py.
But it seems we never use this "absorption_spectra" anywhere, am I wrong? I'll see if I can understand this while I fix the other issues you mentioned.
There was a problem hiding this comment.
There was a problem hiding this comment.
my bad, I was searching for adsorption_spectra
There was a problem hiding this comment.
Ok, perfect. About the misspell, I had noticed it in your messages but forgot to point it out. In this context, it is always absorption.
|
With the added test data, can you please also add the tests for the yambo parser? |
I have just created a tentative version of a test_yambo_parser.py file for testing spectra parsing, trying to follow the structure of other test parsers in that folder. I am not so confident about it..... As for my /test/data/yambo folder, instead, I had to upload its /SAVE folder zipped due to file size limits, and for the same reason the system does not allow me to unzip it there on github. The SAVE folder (which contains the ns.db file) should not be needed for spectra parsing, but its absence is expected to lead to missing parsing of atom coordinates, chemical composition, cell parameters, which in "production" parsing is unpleasant. If you think we can ignore this for the tests, then I can just delete the /SAVE folder. |
| archive = EntryArchive() | ||
| parser = YamboArchiveWriter() | ||
| parser.parse('tests/data/yambo/r_setup',archive,LOGGER) | ||
| assert spectra_files[0] = 'o-R_methylox_TDLDA.alpha_q1_slepc_alda_bse' |
There was a problem hiding this comment.
The assertion here should be
spectra = archive.data.outputs[0].adsorption_spectra[0]
assert spectra.sp_type == 'Polarizability'
...
There was a problem hiding this comment.
Done, i.e. I have put your lines in place of my previous line on checking spectra type.
I expect the previous line, which should check it finds our spectra file, is ok? There I have spectra_files[0] because, in principle, spectra_file is a list, but in the present case it contains 1 file only.
There was a problem hiding this comment.
there is no spectra_file defined under AdsorptionSpectra only: value, energies, sp_typeand n_energies
| @@ -114,4 +122,4 @@ | |||
There was a problem hiding this comment.
you should also add an annotation for Outputs.m_def here with SPECTRA_KEY something like
add_mapping_annotation(Outputs.m_def, SPECTRA_KEY, '.@')
|
|
||
| add_mapping_annotation(general.Simulation.m_def, OUT_KEY, '@') | ||
| add_mapping_annotation(general.Simulation.m_def, NETCDF_KEY, '@') | ||
|
|
There was a problem hiding this comment.
also for general.Simulation.m_def
There was a problem hiding this comment.
Done (line 125), but for some reason it does not label it as "outdated" here.
|
|
||
| spectra_obj = absorption_spectra_parser.data_object | ||
| spectra_obj.label = sp_type | ||
| spectra_obj.type = SPECTRA_TYPE_MAP.get(sp_type, 'unknown') |
There was a problem hiding this comment.
also, labeland type are not defined in AdsorptionSpectra
There was a problem hiding this comment.
since we would like to use the new definition for AbsorptionSpectrum, this should be: AbsorptionSpectrum.m_def not outputs.AbsorptionSpectrum.m_def
Work in progress to share
l275-304
#start HB
def get_spectra(self) -> dict[str, Any]:
data = []
names = []
l345-373
#start HB
SPECTRA_TYPE_MAP = {
'Absorption': 'dielectric_function',
'EELS': 'energy_loss_spectrum',
}