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

45 add basic structure for energy classes #46

Closed

Conversation

JFRudzinski
Copy link
Collaborator

@JFRudzinski JFRudzinski commented Apr 9, 2024

@ndaelman-hu @JosePizarro3 @Bernadette-Mohr Here is a first proposal for a basic structure for various energies. Please provide feedback at your convenience (you can exclusively look at energies.py)

@JFRudzinski JFRudzinski linked an issue Apr 9, 2024 that may be closed by this pull request
@JFRudzinski
Copy link
Collaborator Author

Thinking now about extending this to forces: Should the forces structure not be an exact mirror of energies? If so, how can we encourage or ensure that they are always updated/extended simultaneously?

@JosePizarro3
Copy link
Collaborator

Thinking now about extending this to forces: Should the forces structure not be an exact mirror of energies? If so, how can we encourage or ensure that they are always updated/extended simultaneously?

Are they tho? I am guessing that forces are matrices (or vectors?), while energies are scalars, no?

What do you mean "updated/extended simultaneously"? We can do a quick chatting in a Discord room to understand this point too.

@ndaelman-hu ndaelman-hu self-requested a review April 10, 2024 09:43
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.

Reviewed outputs.py, can do the rest if needed.
No real conceptual comments, just specifics.

src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
src/nomad_simulations/outputs.py Outdated Show resolved Hide resolved
@JosePizarro3
Copy link
Collaborator

JosePizarro3 commented Apr 10, 2024

Reviewed outputs.py, can do the rest if needed. No real conceptual comments, just specifics.

You are reviewing the wrong changes. The ones in #43 are the ones you want to take a look.

@JFRudzinski
Copy link
Collaborator Author

Thinking now about extending this to forces: Should the forces structure not be an exact mirror of energies? If so, how can we encourage or ensure that they are always updated/extended simultaneously?

Are they tho? I am guessing that forces are matrices (or vectors?), while energies are scalars, no?

What do you mean "updated/extended simultaneously"? We can do a quick chatting in a Discord room to understand this point too.

Sorry for being unclear. I just meant that besides the total force, there are various force contributions defined currently, e.g., "free" and "t0". These directly correspond to some energy contributions (although the schema is not nearly as built out as for energies. In principle, there could be a force contribution/definition for every energy, since they are directly related.

My question was, why not enforce there to always be both terms added, for each type of contribution? And if this is desired how can we try to achieve this in practice?

For the classical contributions I will build this mirroring regardless, since energy and force contributions coming from particular interactions are commonly stored.

@ndaelman-hu
Copy link
Collaborator

Reviewed outputs.py, can do the rest if needed. No real conceptual comments, just specifics.

You are reviewing the wrong changes. The ones in #43 are the ones you want to take a look.

@JFRudzinski if you branch off from a non-merged branch, pls specify this in the PR or set the target branch to the proper base...

@JFRudzinski JFRudzinski changed the base branch from develop to 40-physicalproperty-definition-jmp April 10, 2024 12:48
@JFRudzinski
Copy link
Collaborator Author

Reviewed outputs.py, can do the rest if needed. No real conceptual comments, just specifics.

You are reviewing the wrong changes. The ones in #43 are the ones you want to take a look.

@JFRudzinski if you branch off from a non-merged branch, pls specify this in the PR or set the target branch to the proper base...

Ah! Got it, Sorry about that!

@ndaelman-hu
Copy link
Collaborator

ndaelman-hu commented Apr 10, 2024

My question was, why not enforce there to always be both terms added, for each type of contribution? And if this is desired how can we try to achieve this in practice?

I don't think that we should enforce this across the board for 2 reasons:

  1. to derive a (general) force, you need to know the energy manifold in terms of its dependent variables. It's not a given that this information will always be present.
  2. I'm not sure whether it's always desirable for the (general) "forces" to be known. E.g. in the case of total DFT energy, I have little interest in the chemical potential. This is only relevant from a convergence perspective.

With that said, I think it's a good idea to provide an optional functionality. Would recommend something numpy-based as implementation.

@JFRudzinski
Copy link
Collaborator Author

ide an optional functionality. Would recommend something numpy-based as implementation.

I don't really understand what you are saying tbh. I am not talking about enforcing the storage of such quantities. I was talking about schema development. I think "enforcing" is a bit too strong of a word, but I had in mind the case that someone wants to store some energy contribution and implements a class for it, but doesn't create the corresponding force contribution because they personally do not store this in there simulations. But then another user comes along and wants to store this force contribution and now has to extend the schema themselves, whereas if the original person had been encouraged to add a corresponding force contribution it would have made everyone's life a lot easier.

Of course there might be some energy definitions that it doesn't make sense to define a corresponding force. In that case, obviously it should not be done, but I think in most cases it is possible at least in principle.

Base automatically changed from 40-physicalproperty-definition-jmp to develop April 11, 2024 11:29
@JFRudzinski JFRudzinski force-pushed the 45-add-basic-structure-for-energy-classes branch from 7bfc4e9 to d7cd51f Compare April 11, 2024 13:56
@JosePizarro3
Copy link
Collaborator

To be honest, I need to see or talk about an example to fully understand the situation. But I'd wildly suggest to use physical_property_ref (without setting it to is_derived == True) for these cases.

But again, I need to see the example in a board or in a piece of paper.

@ndaelman-hu
Copy link
Collaborator

ndaelman-hu commented Apr 12, 2024

ide an optional functionality. Would recommend something numpy-based as implementation.

I don't really understand what you are saying tbh. I am not talking about enforcing the storage of such quantities. I was talking about schema development. I think "enforcing" is a bit too strong of a word, but I had in mind the case that someone wants to store some energy contribution and implements a class for it, but doesn't create the corresponding force contribution because they personally do not store this in there simulations. But then another user comes along and wants to store this force contribution and now has to extend the schema themselves, whereas if the original person had been encouraged to add a corresponding force contribution it would have made everyone's life a lot easier.

Of course there might be some energy definitions that it doesn't make sense to define a corresponding force. In that case, obviously it should not be done, but I think in most cases it is possible at least in principle.

Ow, it sounded like you meant the actual storage, hence my reply.

As for schema extension, while the scenario you mention is indeed suboptimal, how would you enforce your suggestion?
We can ask them to pretty-please follow this guideline, either in the README and/or class description. For the latter, they should copy that instruction over. Or, we couple both properties together in their own ArchiveSection. Not sure how the actual implementation would look like.

@JosePizarro3
Copy link
Collaborator

@JFRudzinski bear in mind I merged something which created the sub-folder, properties/energies.py, with FermiLevel and ChemicalPotential defined in there. When you work again on this, you can use that then.

@JFRudzinski
Copy link
Collaborator Author

@JFRudzinski bear in mind I merged something which created the sub-folder, properties/energies.py, with FermiLevel and ChemicalPotential defined in there. When you work again on this, you can use that then.

Yes, got it, thank you 👍

jrudz and others added 6 commits June 12, 2024 17:04
@JFRudzinski JFRudzinski force-pushed the 45-add-basic-structure-for-energy-classes branch from d7cd51f to 2f025fb Compare June 12, 2024 15:04
@JosePizarro3
Copy link
Collaborator

@JFRudzinski to avoid confusion I am closing this in favor of #91

You can add (in the description) a "Closes #numberofissue" so Github closes automatically the issue(s) related with #91

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 basic structure for energy classes
3 participants