Skip to content
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

Open
Mateasek opened this issue Feb 22, 2021 · 6 comments
Open

Class for managing lists holding same types #272

Mateasek opened this issue Feb 22, 2021 · 6 comments

Comments

@Mateasek
Copy link
Member

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.

@vsnever
Copy link
Member

vsnever commented Feb 22, 2021

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 autowrap_function1d() for the Function1D objects more than type checking in the ModelManager.

@CnlPepper
Copy link
Member

CnlPepper commented Feb 22, 2021

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.

@Mateasek
Copy link
Member Author

@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.

@jacklovell
Copy link
Member

Maybe I'm missing something here, but why do we need ModelManagers at all? What's wrong with a standard Python list? The existing ModelManager implementations seem to just be wrappers around Python lists anyway, and lists are fast to work with even in Cython. Even looping over the managers is going through the Python layer, as it uses the__iter__ special method. And Cython should type check each element that's retrieved for you if you specify the type of the variable you assign the element to first, as is already done in the beam and plasma objects.

Before we invest any time trying to better list, I'd like to know what benefit dedicated ModelManager classes have over a list. Perhaps getting rid of them entirely and reducing code complexity would actually be more useful.

@Mateasek
Copy link
Member Author

Mateasek commented Mar 3, 2021

This is actually quite a good comment. I think the only advantage except of checking the type of items on addition is the notifier.

@Mateasek
Copy link
Member Author

Mateasek commented Mar 3, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants