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

added previous ff schema from older version #114

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

JFRudzinski
Copy link
Collaborator

No description provided.

@JFRudzinski JFRudzinski linked an issue Sep 10, 2024 that may be closed by this pull request
@JFRudzinski JFRudzinski force-pushed the 113-add-force-field-schema branch from 18f3091 to bbf213c Compare September 12, 2024 07:42
@coveralls
Copy link

coveralls commented Sep 12, 2024

Pull Request Test Coverage Report for Build 11178486979

Details

  • 237 of 297 (79.8%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 80.022%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/numerical_settings.py 7 8 87.5%
src/nomad_simulations/schema_packages/force_field.py 226 285 79.3%
Totals Coverage Status
Change from base Build 10811203881: -0.03%
Covered Lines: 2227
Relevant Lines: 2783

💛 - Coveralls

@JFRudzinski
Copy link
Collaborator Author

@Bernadette-Mohr You could already take a look at this. I have finished implementing bond potentials, and also added a more general test suite that may be useful to you in other contexts.

I will continue expanding to the other potential types, but let me know if you have any major feedback about the direction this is going.

@JFRudzinski
Copy link
Collaborator Author

@JosePizarro3 @ndaelman-hu @EBB2675 @Bernadette-Mohr FYI - I implemented a function here in test_force_field.py for comparing 2 nested dictionaries, that deals with some typical data types that we have in the archive. This way I just take a section that I populated in the archive and do <section>.m_to_dict()>, and then compare with the expected results that I input in a pytest fixture.

I'm sure it could be improved, and maybe you already have such tools in your testing, but perhaps we can collect these into one file in tests/ so that devs can use them, and it encourages consistency in testing.

Also, I am definitely open to feedback concerning the structure of the tests.

@ndaelman-hu
Copy link
Collaborator

ndaelman-hu commented Sep 20, 2024

@JosePizarro3 @ndaelman-hu @EBB2675 @Bernadette-Mohr FYI - I implemented a function here in test_force_field.py for comparing 2 nested dictionaries, that deals with some typical data types that we have in the archive. This way I just take a section that I populated in the archive and do <section>.m_to_dict()>, and then compare with the expected results that I input in a pytest fixture.

I'm sure it could be improved, and maybe you already have such tools in your testing, but perhaps we can collect these into one file in tests/ so that devs can use them, and it encourages consistency in testing.

Also, I am definitely open to feedback concerning the structure of the tests.

Regarding assert_dict_equal, perhaps it's easier if you list out the kind of comparison you want?
Python comes with a native implementation of == for most data structures, including dict:

>> {'a': {'b': 2}, 'c': 3} == {'a': {'b': 2}, 'c': 3}
True

So, if all elements have to match in value (but not necessarily order), you can use the out-of-the-box definition.

If instead only a certain numerical threshold is needed, or only certain properties should be compared, I'd recursively apply a transformation / filtering to the dicts before comparison.
If you also want to perform such comparisons on the matching Sections too (e.g. as with ModelSystem), then I'd overwrite its __eq__.

P.s.: that isn't to say that your implementation is bad, just that it may already be covered. In Haskell, your approach would be called defining the initial algebra to an F-algebra. Given the nice maths of F-algebras, it's quite popular. For initial algebras constructed from algebraic data types, Eq can be derived algorithmically. While recursion is far less powerful in Python, it essentially uses the same trick here.

@JFRudzinski
Copy link
Collaborator Author

@ndaelman-hu thanks for your feedback!

I did originally try to use the native python comparisons, the original function I implemented a while ago, but I believe I could not simply compare 2 dictionaries with arbitrarily nested structures and containing various and a priori unknown types...(I will have to take a look again)

Maybe I over complicated things now, but a couple examples that I needed to treat in a special way:

  1. floats need to be compared with the approx function, and arrays of floats with np.isclose()
  2. np.inf is stored as a string in the archive -- I realize now that this is more of a problem with how I managed the data that I was comparing

I guess these things fall under: "If instead only a certain numerical threshold is needed, or only certain properties should be compared, I'd recursively apply a transformation / filtering to the dicts before comparison." -- what exactly do you mean by this?

…lated missing forces or potentials automatically
@ndaelman-hu
Copy link
Collaborator

@ndaelman-hu thanks for your feedback!

I did originally try to use the native python comparisons, the original function I implemented a while ago, but I believe I could not simply compare 2 dictionaries with arbitrarily nested structures and containing various and a priori unknown types...(I will have to take a look again)

Maybe I over complicated things now, but a couple examples that I needed to treat in a special way:

1. floats need to be compared with the `approx` function, and arrays of floats with `np.isclose()`

2. np.inf is stored as a string in the archive -- I realize now that this is more of a problem with how I managed the data that I was comparing

I guess these things fall under: "If instead only a certain numerical threshold is needed, or only certain properties should be compared, I'd recursively apply a transformation / filtering to the dicts before comparison." -- what exactly do you mean by this?

Indeed, in that case I'd use a recursive function, much as you are doing now, but with the type signature dict -> dict.
In that recursive function, I'd just catch the cases you mentioned above, convert them and leave the rest intact.

There shouldn't be any issue overwriting the values in-place, given that the object where m_to_dict originated from is stored separately. Else, you can use deepcopy.

Lastly, Python has a max. stack frames (1000). If you encounter a stack overload, feel free to reach out, and we look into it.

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 Force Field Schema
3 participants