-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Class for managing lists holding same types #272
Comments
I agree that this might be handy. Although, in particular case of PR #246 the contents of the lists do not change after initialisation, and I like the use of |
In their current forms the model managers can't be made into generic tools as the cython types cannot be dynamically changed. That said, I'm not sure why I wrote the managers in cython. There is probably scope for a clean up, but it isn't really a priority. |
@vsnever and @CnlPepper I made a draft PR #273. This is what I had in mind when proposing to have some class replacing the ModelManagers. It is basically the same, the list handling methods are python and there are 2 cpdef methods for getting the list and its length for faster approach in cython. |
Maybe I'm missing something here, but why do we need Before we invest any time trying to better list, I'd like to know what benefit dedicated |
This is actually quite a good comment. I think the only advantage except of checking the type of items on addition is the notifier. |
The advantage of checking the type on addition can provide clearer debugging. The manager will tell clearly the user he/she tried to add wrong type. This might be useful for less skilled and experienced users. The type checks on addition can be added separately to plasma, beams, lasers, etc. but then you would be repeating what the manager does, wouldn't you? The notifier could be useful for updating caches. I am not aware of any cherab class doing it now, but I can imagine it could be useful in the future. For example in the case of beams, you don't have to rebuild the whole geometry when models are changed, you only update the list in the material. |
ModelManagers are now being used in several places as beam node and plasma node. Now the same could be use in the Zeeman lineshape models (PR #246 ) and also is used in the upcoming Thomson scattering (PR #219 ). Shouldn't the ModelManager class be added to tools, so it can be reused instead of defined every time? The type of the objects in the list could be a mandatory parameter of the init.
The text was updated successfully, but these errors were encountered: