Replace UUIDS with unique_names #18
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
It is not clear to me where and how it is ensured that a |
In the Graph.py file under the add_vertex method, this method is called whenever an EasyScience object is created. It is the method which actually adds the reference to the object to the global map |
…ience into replace_UUIDS
src/easyscience/global_object/map.py
Outdated
| if class_name not in self._name_iterator_dict.keys(): | ||
| self._name_iterator_dict[class_name] = 0 | ||
| else: | ||
| self._name_iterator_dict[class_name] += 1 |
There was a problem hiding this comment.
or just
self._name_iterator_dict.setdefault(class_name, 0)
self._name_iterator_dict[class_name] += 1avoiding conditionals
There was a problem hiding this comment.
This is indeed better. Changing this.
src/easyscience/global_object/map.py
Outdated
| extracted_list = [] | ||
| for key, item in self.__type_dict.items(): | ||
| if obj_type in item.type: | ||
| extracted_list.append(key) | ||
| return extracted_list |
There was a problem hiding this comment.
or just
return [key for key, item in self.__type_dict.items() if obj_type in item.type]There was a problem hiding this comment.
Yes, list comprehension is indeed better. This was not a part of my PR and the method might be completely removed or changed later on, but lets change it for now.
| @unique_name.setter | ||
| def unique_name(self, new_unique_name: str): | ||
| """ Set a new unique name for the object. The old name is still kept in the map. | ||
|
|
There was a problem hiding this comment.
so we are not able to reuse the unique names then?
why not delete (prune) the vertex? Leaving old mapping seems wasteful.
There was a problem hiding this comment.
We are, but only once the old object using the unique_name has been deleted and garbage collection has run and thus removed its entries from the map.
Alternatively the user can manually prune the old unique_name from the map. The prune method is not "hidden".
We want to encourage users to not reuse unique_names, as that makes them not really "unique".
But maybe this discussion belongs in the ADR Suggestion #16.
| raise TypeError("Unique name has to be a string.") | ||
| self._unique_name = new_unique_name | ||
| self._global_object.map.add_vertex(self) | ||
|
|
There was a problem hiding this comment.
You have 3 identical methods unique_name and _unique_name_generator in
- Variable
- ObjectClasses
- descriptor_base
Since all of them inherit from a base class, which eventually is ComponentSerializer, maybe it would make sense to combine some of these duplicates?
There was a problem hiding this comment.
It would, it would in general make a lot of sense for us to fix our inheritance tree for stuff such as this. I would claim that this does not belong in a ComponentSerializer class, as that class is just for serialization, but rather in a base class which all our objects inherit from, which we don't currently have, as our descriptors and parameters do not inherit from BasedBase.
So this will hopefully be amended in a future PR, when we actually fix our inheritance tree.
| @@ -164,18 +167,19 @@ def virtualizer(obj: BV) -> BV: | |||
| d = obj.encode_data() | |||
| if hasattr(d, 'fixed'): | |||
| d['fixed'] = True | |||
| d['unique_name'] = None | |||
| v_p = cls(**d) | |||
There was a problem hiding this comment.
v_p looks awfully cryptic, can't we rename this to something more meaningful?
There was a problem hiding this comment.
Sure, feel free to change it in a separate branch/PR. I don't have the bigger picture of what this part of the code is actually used for. I think v_p stands for "virtual_parameter", possibly?
src/easyscience/__init__.py
Outdated
| global_object = GlobalObject() | ||
| global_object.instantiate_stack() | ||
| global_object.stack.enabled = False | ||
| borg = global_object # alias for global_object, remove later |
There was a problem hiding this comment.
this assignment could be made a property, which returns global_object but also notifies the user (print()? log.warning()?) about looking obsolescence of borg.
Could be useful when porting the libraries
There was a problem hiding this comment.
I am not sure we even need anything in hugger.
This was going to be an impelentation of some sort of Variable wrapper so all changes to it are saved on the Undo stack.
I don't think it is fully/at all enabled so maybe we should make it go obsolescent.
There was a problem hiding this comment.
I would love to remove it, but unfortunately, it is used, specifically in the logger. Maybe we should create an issue on this to see if it can be changed in the future?
No description provided.