Add BandGap output #53
Conversation
Added iri to quantity in PhysicalProperty
Pull Request Test Coverage Report for Build 8734344240Details
💛 - Coveralls |
|
@JFRudzinski what do you think about creating a sub-folder with the specific physical properties modules? Like I did here for the ElectronicBandGap. |
Not sure if there are any best practices in this direction, but personally, yeah, I think that is quite nice. I like separating out related properties a bit so none of the files become too too large to manage, and then in that case it definitely helps organizationally to have the subfolder 🙌 |
Fix __setattr__ in PhysicalProperty
|
@ndaelman-hu being a band gap, would you mind reviewing this quickly? I think it is very easy, I just had to fix a bit the logic of the setattr in PhysicalProperty. I'd like to merge this today, if there are not too many changes. |
|
Okay, I replied to unresolved (and some resolved) threads. Let's just work through those points, the rest is fine. |
Added raise ValueError in PhysicalProperty when value is None
JosePizarro3
force-pushed
the
31-add-bandgap-output
branch
from
1643440 to
ae7c744
Compare
|
Ok, I added a new method in My reasoning for this is that the I also changed the negative |
| def extract_spin_polarized_property(self, property_name: str) -> list: | ||
| """ | ||
| Extracts the spin-polarized properties if present from the property name and returns them as a list of two elements in | ||
| which each element refers to each `spin_channel`. If the return list is empty, it means that the simulation is not | ||
| spin-polarized (i.e., `spin_channel` is not defined). | ||
|
|
||
| Args: | ||
| property_name (str): The name of the property to be extracted. | ||
|
|
||
| Returns: | ||
| (list): The list of spin-polarized properties. | ||
| """ | ||
| spin_polarized_properties = [] | ||
| properties = getattr(self, property_name) | ||
| for prop in properties: | ||
| if prop.spin_channel is None: | ||
| continue | ||
| spin_polarized_properties.append(prop) | ||
| return spin_polarized_properties | ||
|
|
There was a problem hiding this comment.
This is fine, but note that there's a distinction between a calculation that allowed for spin-relaxation and one where the properties differ between spin-channel.
I think that the former is already covered well enough in model_system.py.
I would use the latter definition here, though: if the DOS up / down are the same, I don't consider it polarized (magnetic).
There was a problem hiding this comment.
Still, both spin-channels are reported. This distinction into the specific magnetic state should come from another property. If the proper occupation per spin channel is not defined, how can you ensure one or another type of magnetic state?
Change variables.Energy to Energy2 to avoid bugs with Normalizers (to be fixed)
|
Ok, I am merging this, but @ndaelman-hu: would you mind if we move this discussion of the magnetic state to #22? I think there we can keep talking + we can eventually invite Hongbin and the postdoc responsible for data to join the discussion 🙂 |
Closes #31