Generic interface for Mesh#326
Conversation
Current coverage is 85.62% (diff: 93.46%)
|
| """ | ||
|
|
||
| @abstractmethod | ||
| def has(self, uid): # pragma: no cover |
There was a problem hiding this comment.
I suggest to implement __contains__ instead of this.
https://docs.python.org/3/reference/datamodel.html#object.__contains__
There was a problem hiding this comment.
However, the same applies to all the other pythonic container methods. Perhaps it's better to leave this the subject of another PR. (And I see we have has_something methods there)
There was a problem hiding this comment.
well, if you do so, it's certainly possible, but at that point we should just reimplement the dict() interface... :)
There was a problem hiding this comment.
Thanks! However, we have to attack the problem of being a dict or not soon...
| """ | ||
|
|
||
| @abstractmethod | ||
| def count_of(self, item_type): # pragma: no cover |
There was a problem hiding this comment.
Shell we implement __len__ beside this? Of course it will return the whole number of elements inside the dataset.
There was a problem hiding this comment.
That's possible, yes.
| from abc import ABCMeta, abstractmethod | ||
|
|
||
|
|
||
| class ABCDataset(object): |
There was a problem hiding this comment.
DataSets must conform the the structore of CUBA.DATA_SET, i.e. they need uid, name, etc defined.
There was a problem hiding this comment.
Ok then I probably used the wrong name. I just picked dataset but I am not sure about it.
What is the generic name for things like particles, mesh, and lattice?
There was a problem hiding this comment.
Do you suggest that I add these as well, or that I should choose another name? This is just an interface to the generic interface, so I am not sure if we want to name it after the Dataset.
There was a problem hiding this comment.
I think the name is fine. How about adding meta.DataSet as a parent to get the properties?
| """ | ||
| raise NotImplementedError("Remove is not implemented for Mesh") | ||
|
|
||
| def iter(self, uids=None, item_type=None): |
There was a problem hiding this comment.
Docstring for item_type is missing
|
@mehdisadeghi please re-review. |
|
Another source of problem is that with the new hierarchy involving the generated base meta classes, the data object now contains None entries, that are converted to the string "None" when written on the hdf5 file with pytables. |
|
The problem of None should be solved by setting appropriate defaults to strings, as from PR #343. In any case, the base classing will be done on a separate PR. |
|
@stefanoborini since you have made separated PRs for bad default values (#343) in the schema and the copy vs. reference |
This PR introduces the generic interface for datasets and applies it to the Mesh object, with minimal disruption to the internals and no disruptions to the old interface.
We expect a loss of efficiency but that's the price to pay for not knowing the uid->type association. At the moment, it's probably not wise to implement optimization strategies to solve this issue, and will be part of a separate PR once we migrated all objects.