Skip to content

Optionally allow None for missing pseudos in a family#193

Closed
edan-bainglass wants to merge 3 commits intoaiidateam:mainfrom
edan-bainglass:flexible-pseudo-fetching
Closed

Optionally allow None for missing pseudos in a family#193
edan-bainglass wants to merge 3 commits intoaiidateam:mainfrom
edan-bainglass:flexible-pseudo-fetching

Conversation

@edan-bainglass
Copy link
Copy Markdown
Member

@edan-bainglass edan-bainglass commented Aug 4, 2025

At the moment, using PseudoPotentialFamily.get_pseudos() will fail for all if at least one element does not have a corresponding pseudopotential in the family. This PR aims to allow a path for partial results, returning None for those missing elements, if user opt in by way of an optional none_if_missing parameter (False by default for backwards compatibility).

This allows the QE app, for example, to do this:

image

@edan-bainglass edan-bainglass force-pushed the flexible-pseudo-fetching branch from b305fc6 to 411ffd7 Compare August 4, 2025 16:12
@edan-bainglass
Copy link
Copy Markdown
Member Author

edan-bainglass commented Aug 4, 2025

@t-reents @mbercx no rush on this and also okay to drop. I realized in the app I can just replace get_pseudos() with a loop over get_pseudo() and try/except missing pseudos, replacing them with None. So this API is not strictly required.

I'll let you decide if this should close or merge. I'm good either way.

@t-reents
Copy link
Copy Markdown
Collaborator

t-reents commented Aug 4, 2025

True, that's a solution as well!

As I told you in person, I was thinking that this change is fine, as it's backwards compatible and having in mind the use-case in the app.
In my opinion, the use-case in the app is/was the strongest motivation for this change. For a normal user, I still feel that it makes sense to raise in case a pseudo is missing, since one is explicitly trying to get the pseudos for a given structure/set of elements from a PseudoFamily (even though I also see the other side to provide at least the available pseudos).

In summary, no strong opinion at my end but a preference towards the current behavior. I'd leave it up to @mbercx to make a final decision

@edan-bainglass
Copy link
Copy Markdown
Member Author

Alternative solution working as expected. Closing this.

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