Skip to content

Added Fermi model, denoising function, dos alignment function, DOS an…#144

Merged
HowWeiBin merged 49 commits into
mainfrom
dos
Jun 12, 2026
Merged

Added Fermi model, denoising function, dos alignment function, DOS an…#144
HowWeiBin merged 49 commits into
mainfrom
dos

Conversation

@HowWeiBin

Copy link
Copy Markdown
Contributor

These are the functionalities that were requested to enhance the cookbook example. As the timeline for the cookbook is quite tight, please approve/tell me if more changes are needed ASAP

…d mask calculation from eigenvalues, and dos and mask padding for training
@HowWeiBin HowWeiBin requested a review from abmazitov May 13, 2026 14:42
@HowWeiBin HowWeiBin self-assigned this May 13, 2026
Comment thread src/upet/calculator.py Outdated
Comment thread src/upet/calculator.py Outdated
Comment thread src/upet/calculator.py Outdated
@abmazitov

Copy link
Copy Markdown
Collaborator

Thanks a lot @HowWeiBin ! A few comments:

  1. Is there any reason to use the DOS calculation without a denoising algorithm? If it's always better and more physical to use denoising, why don't we always work with a denoised DOS and get rid of a default non-denoised setting? IMO having two settings where one is clearly told to be worse than another seems a bit confusing from a user perspective.
  2. The model=True option for computing the bandgap actually falls into the same problem - why don't we always use a dedicated CNN to compute the bandgap if it's proven to be always better? What's the benefit of getting it directly from the DOS?

@ceriottm

Copy link
Copy Markdown
Contributor

Thanks a lot @HowWeiBin ! A few comments:

  1. Is there any reason to use the DOS calculation without a denoising algorithm? If it's always better and more physical to use denoising, why don't we always work with a denoised DOS and get rid of a default non-denoised setting? IMO having two settings where one is clearly told to be worse than another seems a bit confusing from a user perspective.
  2. The model=True option for computing the bandgap actually falls into the same problem - why don't we always use a dedicated CNN to compute the bandgap if it's proven to be always better? What's the benefit of getting it directly from the DOS?

I'd be in favor of keeping the options here. Both of these are tricks to address very large errors, but make the predictions inconsistent with each other. It could very well be that when fine tuning denoising or gap head become far less preferable, and then we'd have to make a change, and we'd be discussing how we keep changing the option set of the models.

@ceriottm

Copy link
Copy Markdown
Contributor

I shared some design comments offline with @HowWeiBin . I should add here that I'd call compute_DOS_and_mask_from_eigenvalues -> dos_from_eigenvalues and pad_dos_and_mask_for_training as pad_dos (and as discussed offline, make generic versions available from utils so that they can be called without having a trained model already.

@HowWeiBin

Copy link
Copy Markdown
Contributor Author

@abmazitov and @pfebrer I have modified the code to fit the changes in the loss function in metatrain. Please review! Pol and I have removed the energy grids and we made the entire process agnostic to the energy reference

Comment thread docs/src/usage/ase.rst Outdated
Comment thread docs/src/usage/ase.rst Outdated
Comment thread docs/src/usage/ase.rst Outdated
Comment thread docs/src/usage/ase.rst Outdated
Comment thread docs/src/usage/ase.rst Outdated
Comment thread src/upet/calculator.py Outdated
Comment thread src/upet/calculator.py Outdated
Comment thread tests/pet_mad_dos/test_pet_mad_dos_basic_usage.py Outdated
@pfebrer

pfebrer commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Otherwise I think that now things are much better so it looks good to me 👍

@HowWeiBin

Copy link
Copy Markdown
Contributor Author

all tests passed! @abmazitov anything from your end?

Corrected the spacing in the units for DOS intervals.
Corrected spacing in units description for DOS intervals.

@abmazitov abmazitov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thank you @HowWeiBin !

@HowWeiBin HowWeiBin merged commit 4fd85a8 into main Jun 12, 2026
8 checks passed
@HowWeiBin HowWeiBin deleted the dos branch June 12, 2026 10:50
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.

4 participants