Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DielectricFunction properties #72

Merged
merged 7 commits into from
May 28, 2024

Conversation

JosePizarro3
Copy link
Collaborator

@JosePizarro3 JosePizarro3 commented May 21, 2024

Another property added to the list. In this case, there is a renaming to Permittivity to be in line with the taxonomy. I have to check if we can define DielectricStrength, and how to extract it from Permittivity.

The design idea here is that we want to store a 3x3 tensor $\epsilon(q, \omega)$. If it does not depend on $q$, we can extract the absorption spectrum as the imaginary term like $I(\omega) = Im \epsilon(\omega)$. Like the spectrum has a rank=[], and the permittivity has rank=[3, 3], I just needed to create 3 sub-sections for each principal direction for the spectrum.

I also:

  • Added a new utils function to extract the specific list of variables (function get_variables()).
  • Added abstraction for spectrum, defining AbsorptionSpectrum as a more abstract category for XAS, EELS. I added axis as a MEnum
  • I had to add KMesh (from KSpace definition #71), and I also defined Frequency for variables
  • Added Permittivity.type for simpler categorization

@JosePizarro3 JosePizarro3 self-assigned this May 21, 2024
@JosePizarro3 JosePizarro3 linked an issue May 21, 2024 that may be closed by this pull request
@JosePizarro3
Copy link
Collaborator Author

Hi @ndaelman-hu, @ladinesa

I am not 100% who is better for reviewing this: I don't want comments on the schema, but rather on the implementation of functionalities and check that the testing is correct.

Let me know.

@coveralls
Copy link

coveralls commented May 21, 2024

Pull Request Test Coverage Report for Build 9269207816

Details

  • 102 of 102 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 98.616%

Totals Coverage Status
Change from base Build 9254335937: 0.1%
Covered Lines: 784
Relevant Lines: 795

💛 - Coveralls

Copy link
Collaborator

@ndaelman-hu ndaelman-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only checked the get_variables utility, as you asked.
Check whether it's really worth having it as a utility and if so, the edge cases I mentioned.

I think this shows 1 weakness of the naive flat schema, is that we need these scans to provide lookup. For small archives, there's no real issue, but I'm unsure about search at the DB level for 2 reasons:

  1. the schema does not serialize the actual class. Will we be able to read in more specialized child classes?
  2. I don't know how an ES query will perform on these lists. Actually, do you have any tag to look for?

These 2 questions are more general than this PR. Just something th at I noticed.

src/nomad_simulations/utils/utils.py Show resolved Hide resolved
src/nomad_simulations/utils/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
src/nomad_simulations/variables.py Outdated Show resolved Hide resolved
src/nomad_simulations/utils/utils.py Show resolved Hide resolved
@ndaelman-hu
Copy link
Collaborator

Hi @ndaelman-hu, @ladinesa

I am not 100% who is better for reviewing this: I don't want comments on the schema, but rather on the implementation of functionalities and check that the testing is correct.

Let me know.

Did a small review. Btw, you can also ask @Bernadette-Mohr. I think she's more than capable providing feedback on the utility.

@JosePizarro3
Copy link
Collaborator Author

I only checked the get_variables utility, as you asked. Check whether it's really worth having it as a utility and if so, the edge cases I mentioned.

I think this shows 1 weakness of the naive flat schema, is that we need these scans to provide lookup. For small archives, there's no real issue, but I'm unsure about search at the DB level for 2 reasons:

  1. the schema does not serialize the actual class. Will we be able to read in more specialized child classes?
  2. I don't know how an ES query will perform on these lists. Actually, do you have any tag to look for?

These 2 questions are more general than this PR. Just something th at I noticed.

Not sure if I understand the point of the weakness. This data structure only stores the data, and in case of talking about querying, I doubt searching for specific variables would ever be a topic (but rather, that a property has a variable_cls defined).

Actually, do you have any tag to look for?

Again, I am never talking about ES in this schema.

@JosePizarro3
Copy link
Collaborator Author

Hi @ndaelman-hu, @ladinesa
I am not 100% who is better for reviewing this: I don't want comments on the schema, but rather on the implementation of functionalities and check that the testing is correct.
Let me know.

Did a small review. Btw, you can also ask @Bernadette-Mohr. I think she's more than capable providing feedback on the utility.

Thanks! I will incorporate your feedback and merge. I think it makes sense if I actually merge after #71

@ndaelman-hu
Copy link
Collaborator

I only checked the get_variables utility, as you asked. Check whether it's really worth having it as a utility and if so, the edge cases I mentioned.
I think this shows 1 weakness of the naive flat schema, is that we need these scans to provide lookup. For small archives, there's no real issue, but I'm unsure about search at the DB level for 2 reasons:

  1. the schema does not serialize the actual class. Will we be able to read in more specialized child classes?
  2. I don't know how an ES query will perform on these lists. Actually, do you have any tag to look for?

These 2 questions are more general than this PR. Just something th at I noticed.

Not sure if I understand the point of the weakness. This data structure only stores the data, and in case of talking about querying, I doubt searching for specific variables would ever be a topic (but rather, that a property has a variable_cls defined).

Actually, do you have any tag to look for?

Again, I am never talking about ES in this schema.

You mentioned before essentially copying a filtered version of this schema over into results, that's why I'm asking.
If so, we should consider the points I raised, though we can discuss it outside of this PR. If searching for variable_cls is supported, my point 2 remains.

@JosePizarro3
Copy link
Collaborator Author

I only checked the get_variables utility, as you asked. Check whether it's really worth having it as a utility and if so, the edge cases I mentioned.
I think this shows 1 weakness of the naive flat schema, is that we need these scans to provide lookup. For small archives, there's no real issue, but I'm unsure about search at the DB level for 2 reasons:

  1. the schema does not serialize the actual class. Will we be able to read in more specialized child classes?
  2. I don't know how an ES query will perform on these lists. Actually, do you have any tag to look for?

These 2 questions are more general than this PR. Just something th at I noticed.

Not sure if I understand the point of the weakness. This data structure only stores the data, and in case of talking about querying, I doubt searching for specific variables would ever be a topic (but rather, that a property has a variable_cls defined).

Actually, do you have any tag to look for?

Again, I am never talking about ES in this schema.

You mentioned before essentially copying a filtered version of this schema over into results, that's why I'm asking. If so, we should consider the points I raised, though we can discuss it outside of this PR. If searching for variable_cls is supported, my point 2 remains.

I am still not sure. In results there will be a reference to the specific property (e.g., permittivity). The variables will never be searched, but probably it will be filtered to the result which variables we want to make searchable. This is as optimal as whatever is already in place for the results section when it is referencing properties in run.

Maybe I used wrong the word "copy"?

@JosePizarro3 JosePizarro3 force-pushed the 34-add-dielectricfunction-properties branch from 81da0d9 to 74532b9 Compare May 22, 2024 11:21
@JosePizarro3 JosePizarro3 changed the base branch from develop to 69-kspace-definition May 22, 2024 11:21
@JosePizarro3 JosePizarro3 force-pushed the 34-add-dielectricfunction-properties branch from 9320621 to dc92720 Compare May 22, 2024 12:29
@JosePizarro3 JosePizarro3 force-pushed the 69-kspace-definition branch from d657e14 to a9bdac2 Compare May 22, 2024 12:38
Base automatically changed from 69-kspace-definition to develop May 27, 2024 11:53
Added KLinePath and KMesh variables with refs to the NumericalSettings sections
@JosePizarro3 JosePizarro3 force-pushed the 34-add-dielectricfunction-properties branch from dc92720 to c145924 Compare May 27, 2024 15:00
…existing in the previous commit when rebasing. what-the-actual-fuck ... -.-
@JosePizarro3
Copy link
Collaborator Author

I think I will be merging this tomorrow morning! No problem, we can revisit this as well in the future.

@JosePizarro3 JosePizarro3 merged commit 4968164 into develop May 28, 2024
4 checks passed
@JosePizarro3 JosePizarro3 deleted the 34-add-dielectricfunction-properties branch May 28, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DielectricFunction properties
3 participants