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

move to _model.py naming scheme for base objects which are subclassed (datablock, multiblock, naparidepictor) #74

Closed
alisterburt opened this issue Mar 5, 2021 · 7 comments

Comments

@alisterburt
Copy link
Collaborator

I like the pattern used in napari etc for naming these things 'viewer_model', 'layer_model' etc. It clearly separates the abstract from the concrete implementations wthout needing to explicitly write abstract base classes

Why did we move away from using abstract base classes for these in the first place?

@alisterburt alisterburt changed the title move to _model.py for base objects which are subclassed (datablock, multiblock, naparidepictor) move to _model.py naming scheme for base objects which are subclassed (datablock, multiblock, naparidepictor) Mar 5, 2021
@brisvag
Copy link
Owner

brisvag commented Mar 5, 2021

Im not sure about this; isn't the whole point of "models" in napari to separate the logic from specifically the GUI? AFAIK the whole model thing is simply a way to implement the event loop and all the logic related to it without depending on the qt frontend. Am I wrong?

As for our case: there was really no reason to keep the abstract classes, since it added nothing. The only use it had was to enforce the implementation of _data_setter in subclasses, which as easily done by raising NotImplementedError and requires less importing and fewer lines of code.

Where do you see a need/benefit of using abstract classes and/or "models"?

@alisterburt
Copy link
Collaborator Author

Generally I just like the abstract class pattern, my feelings pretty much align with what this person thinks in that it's just the right object for the job (we never instantiate datablocks directly)

I'm not excessively bothered by it though and happy to go either way!

For the models, you might be right about the event loop stuff but more broadly I like the idea of it being obvious where all of the base components are and their implementations separate - for creating this separation there are two solutions

  1. some obvious thing in the filename of the 'DataBlocks' directory, like _model.py
  2. a separate module for components, where all of the abstract base classes for different types of things we work with would live

What're your thoughts?

@brisvag
Copy link
Owner

brisvag commented Mar 11, 2021

I see your point about abstract classes... Maybe that's better! But yeah, function-wise it's not that important.

As for models: I still don't get it... can you give me a practical example for peepingtom? What would you split into a "model" and its "implementation", how roughly?

@alisterburt
Copy link
Collaborator Author

alisterburt commented Mar 11, 2021

Yep sure

The DataBlock class would be a 'model' the way I'm seeing it, either as an abstract class in its own file in a components module or in a datablock_model.py file in the directory with all of the classes which inherit from it. The implementations are then each class which inherits from that base class

@brisvag
Copy link
Owner

brisvag commented Mar 11, 2021

So you're basically saying that we should make DataBlock and other base classes into abstract classes, and those should be called models for clarity?
If yes, this is a completely different thing from how model is defined in napari... It resembles more what they do with layers, where base contains what you are calling models, and the daughter classes are in separate things.

This is what we had at some point, with a base.py in every module, but I don't really like that because it makes the file/module not self-explanatory (same reason why we should probably change all the utils into something better). I think datablock.py is clearly enough the base implementation of all the datablocks.

@alisterburt
Copy link
Collaborator Author

not necessarily changing the name of the class, just indicating in the file name that it's a model - I actually prefer the other option I suggested, having all of the base classes as components in a separate components module, in any case it's not super important just a choice, I like the clear separation :)

@brisvag
Copy link
Owner

brisvag commented May 25, 2021

Addressed this with the addition of abstract classes in #84. Names stayed more or less the same, but they are logically separated by the rest.

@brisvag brisvag closed this as completed May 25, 2021
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

2 participants