Space group cosmetics #230
Conversation
gflownet/envs/crystals/spacegroup.py
Outdated
| @@ -583,7 +600,7 @@ def point_group(self) -> str: | |||
| def get_state_type(self, state: List[int] = None) -> int: | |||
| """ | |||
| Returns the index of the type of the state passed as an argument. The state | |||
| type is one of the following (self.state_type_indices): | |||
| type is one of the following (StateType): | |||
| 0: both crystal-lattice system and point symmetry are unset (== 0) | |||
| 1: crystal-lattice system is set (!= 0); point symmetry is unset | |||
| 2: crystal-lattice system is unset; point symmetry is set | |||
There was a problem hiding this comment.
This docstring needs to be updated to reflect the new possible state.
Also, ideally, the implementation (and docstring) would need to be updated not to rely on the values in the StateType enum having specific numerical values since it defeats the purpose of having an enum in the first place.
There was a problem hiding this comment.
Yeah you are right. Enum does not seem to be the best option then because the specific values of each state type matters as it is part of the actions. I have changed things in bf43239 by not making it an enum anymore, but the ALL variables I have added to iterate over the state types and properties seems pretty ugly. Do you have a better suggestion? Make them just dictionaries? I would consider the possibility of extending the properties in the future (e.g. extend the hierarchy with Bravais lattice, super-space groups, etc.) and hence the state types.
There was a problem hiding this comment.
Can they simply be constants in the SpaceGroup class?
Given that their purpose seems to be to describe the state of a SpaceGroup env, and that the SpaceGroup env relies heavily on the specific values that these will take... it seems like they're so coupled together that my first idea would have been to keep them in the same class.
And then instead of doing StateType.CLS_PS, it's Spacegroup.CLS_PS. Almost identical. But the fact that those constants would be defined within the SpaceGroup class, and not an enum, will make it obvious that you can't just change those without care. The SpaceGroup class requires these specific values.
There was a problem hiding this comment.
They could. My concern with that would just be about the variable name. StateType.CLS_PS is clearer than Spacegroup.CLS_PS or just CLS_PS.
The current implementation (bf43239) is something in between: not an Enum, not in the SpaceGroup class, but a simple class. Would you be ok with that? I am thinking that having a class may come useful for future extensions.
Make missing change
Pure refactoring, no change of functionality.
Builds on top of PR #223.