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

DRAFT: Move periodic table definition to dictionary #138

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

nils-wittemeier
Copy link
Contributor

@nils-wittemeier nils-wittemeier commented Apr 22, 2024

As the title states, this PR moves the definition of the periodic table to a new datatype.

The following change are made:

  • Adds new dependency 'fhash'
  • Adds a new data type isotopes.
  • Moves the definition of isotope masses and abundance to 'params' module
  • Moves definition of precisions (r64, r128, i64) to new module called 'precision'. I took the liberty of splitting this part of params into a new module, to avoid circular imports. Otherwise m_isotopes requires r64 from params and params requires the new datatype isotopes from m_isotopes. It seems like similar issues could arise in the future in other contexts.

Before merge:

  • replace existing queries in old periodic table with queries in the new table
  • remove old table
  • test

  - Adds new dependency 'fhash'
  - Adds a new data type isotopes
  - Moves definition of isotopes to 'params' module
  - Moves definition of precisions to new module 'precision'
@nils-wittemeier
Copy link
Contributor Author

I briefly checked that everything compiles, but had some MPI-related issues when running tests, that I'll have to look into.

@nils-wittemeier
Copy link
Contributor Author

It might be worth thinking of whether quantities like the average mass should be calculated in the constructor of the isotopes type, or whether to implement a function that returns the average mass.

Copy link
Owner

@nakib nakib left a comment

Choose a reason for hiding this comment

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

Dear Nils. Thanks for the contribution, really appreciate it. I left some comments. Would you please consider these? One crucial point: the CI broke since the type check in lookup_periodic_table caught an invalid type. Would be great if you could check this. Thanks a lot!

src/m_isotopes.f90 Outdated Show resolved Hide resolved
src/m_isotopes.f90 Outdated Show resolved Hide resolved
src/crystal.f90 Show resolved Hide resolved
src/params.f90 Show resolved Hide resolved
src/params.f90 Show resolved Hide resolved
src/params.f90 Show resolved Hide resolved
src/precision.f90 Show resolved Hide resolved
@nakib
Copy link
Owner

nakib commented Apr 23, 2024

It might be worth thinking of whether quantities like the average mass should be calculated in the constructor of the isotopes type, or whether to implement a function that returns the average mass.

This is a good point. Since there is now two ways to think about the atomic mass -- virtual crystal approximation (VCA) or dominant isotope background (DIB) https://journals.aps.org/prb/abstract/10.1103/PhysRevB.109.165201 -- it would be good to have one or both of these masses served by someone on demand. Now whether this should be the crystal or the isotopes type is something to think about. I was thinking that isotopes should be a private type inside the crystal_module.

@nils-wittemeier
Copy link
Contributor Author

Thank you for the feedback.

I agree with everything.

I will work on it soon.

Copy link
Owner

@nakib nakib left a comment

Choose a reason for hiding this comment

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

I think I found the issue. You might just have to do call periodic_table%get_raw(key(element), raw_data) -> call periodic_table%get_raw(key(trim(element)), raw_data) in lookup_periodic_table.

src/params.f90 Outdated Show resolved Hide resolved
src/params.f90 Outdated Show resolved Hide resolved
src/crystal.f90 Outdated Show resolved Hide resolved
src/crystal.f90 Outdated Show resolved Hide resolved
src/crystal.f90 Outdated Show resolved Hide resolved
@nils-wittemeier
Copy link
Contributor Author

I think I found the issue. You might just have to do call periodic_table%get_raw(key(element), raw_data) -> call periodic_table%get_raw(key(trim(element)), raw_data) in lookup_periodic_table.

Yes, this is what I suspected. I will check tomorrow and update the branch.

@nakib nakib merged commit a23a172 into nakib:dictionary Apr 25, 2024
1 check passed
@nakib
Copy link
Owner

nakib commented Apr 25, 2024

Nils, everything is in order. Thanks a lot for your contribution.

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.

2 participants