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

BadElfAnalysis classification as PopulationAnalysis #557

Open
jacksund opened this issue Dec 28, 2023 · 2 comments
Open

BadElfAnalysis classification as PopulationAnalysis #557

jacksund opened this issue Dec 28, 2023 · 2 comments
Labels
refactor cleans up code without affecting functionality

Comments

@jacksund
Copy link
Owner

jacksund commented Dec 28, 2023

similar to how bader is a population-analysis type, I think badelf workflows should be too

@jacksund jacksund added the refactor cleans up code without affecting functionality label Dec 28, 2023
@SWeav02
Copy link
Contributor

SWeav02 commented Jan 3, 2024

@jacksund I'm fine with this, but I think we should change the population-analysis table so that it doesn't inherit from the static-energy table. I think in most use cases the users looking for BadELF will have already run the DFT calculation. Thoughts?

@jacksund
Copy link
Owner Author

jacksund commented Jan 3, 2024

yep we can do this. we can have it inherit from Structure instead of StaticEnergy -- or even have PopulationAnalysis as its own basic mix-in class. I'm sure there are population analyses out there that don't involve a static energy calc (maybe like ML predictions).

The counter argument is that the static energy fields are optional and only there if you need them. So at worst there are just empty columns if the user already ran their dft calculation. Still, we'll likely simplify the table like you suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor cleans up code without affecting functionality
Projects
None yet
Development

No branches or pull requests

2 participants