search

AI-multimodal / aimmdb

BSD 3-Clause "New" or "Revised" License
0 stars 10 forks source link

Refactor XDIElement into Element and Edge #21

Closed matthewcarbone closed 2 years ago

matthewcarbone commented 2 years ago

https://github.com/AI-multimodal/aimmdb/blob/673b5ce88ef21ec259c5a262712e58ad9c31e2a6/aimmdb/schemas.py#L83

@kleinhenz as we discussed, unless you think this will break a bunch of stuff, I think we should refactor this into an Edge and Element, two separate classes, if only for readability. What do you think? I'm happy to take a shot at this just for practice.

kleinhenz commented 2 years ago

I think maybe we should discus this a little more before departing from the only existing attempt at schematizing XAS metadata here. Do you think it will it be more or less confusing to have our element mean the equivalent of XDI element.symbol?

matthewcarbone commented 2 years ago

Happy to discuss some more definitely. As I'm sure you're aware at this point I have strong opinions but I'll try to outline the pros and cons as I see them:

The primary improvement from refactoring is readability. Physically, the edge and element are completely different things. The edge corresponds to a location in the energy range of XAS data specifically. So if the data isn't XAS, the edge doesn't really make sense. Separating them will make the code more modular.

As I see it, the disadvantages are (a) breaking with this XDI convention, and (b) possibly breaking the code by making the changes.

Do you think it will it be more or less confusing to have our element mean the equivalent of XDI element.symbol?

I think it's more confusing the way it is now. I'm sure on the XDI side they had their reasons for organizing things this way, but to me it doesn't really make sense to pool the XAS edge into an Element object, since the edge is not a fundamental property of the element, it is a function of the element but only for XAS (and possibly other select spectroscopies that I don't know of).

Thoughts?

matthewcarbone commented 2 years ago

Recommend we either revisit this or just outright close it and proceed continuing the XDIElement object as is. @danielballan thoughts? I know this is based off of some precedent but that doesn't mean it's not a misnomer 😁

danielballan commented 2 years ago

I'd be happiest sticking with the XDI convention for now, while staying open to revisiting this after we have a couple months of experience using it with the whole team.