Base Classes aren't named consistently

[kain88-de]

There is AnalysisBase, Reader, ProtoReader and I'm sure some that I forgot in the topology system and other places. Almost pure Abstract classes like AnalysisBase should follow a consistent naming scheme.

Personally I would like BaseXXX because my fingers always want to type it this pattern but XXXBase is just as fine.

List of abstract base classes

• AnalysisBase
• ProtoReader
• Reader

[richardjgowers]

Yeah a good point while we can still change these things. It is worth remembering that these all live in a base submodule, but coordinates.base.ReaderBase seems like a nice name.

ProtoReader is just an intermediate subclass which we could probably squash out of existence..

[jbarnoud]

Being all in a base.py file, couldn't we just remove the Base part of the name? I prefer base.Analysis rather than base.AnalysisBase that feels redundant. Just a thought, though.

[jdetle]

I move to upgrade Jonathan's thought to a good suggestion.

[richardjgowers]

@jbarnoud I did think the same, but we should probably make the Bases available from the module (not submodule) so coordinates.ReaderBase == coordinates.base.ReaderBase. And second, once things get imported around the base submodule gets lost.

[mnmelo]

I came up with the ProtoReader to have a Reader base class without a __del__, for the cases that don't need one, such as the SingleFrameReader.

If we want to do away with ProtoReader and have only a Reader base class we still need a way to disable __del__ where unneeded. A pythonic alternative would be to have the __del__ implemented in a second base class (say, ReaderCleanup) and have individual subclasses that need __del__ inherit from both Reader and ReaderCleanup.

[richardjgowers]

@mnmelo I'm actually hacking together a Reader that doesn't maintain an open file handle.. so hopefully that whole __del__ issue becomes moot :)

[mnmelo]

@richardjgowers, perfect!

[jbarnoud]

On 03/08/16 19:44, Richard Gowers wrote:

@jbarnoud https://github.com/jbarnoud I did think the same, but we
should probably make the Bases available from the module (not
submodule) so |coordinates.ReaderBase == coordinates.base.ReaderBase|.
And second, once things get imported around the |base| submodule gets
lost.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#921 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUWujP-Dfll8NziDJ2vZC6-iABt8yZkks5qcNNhgaJpZM4JZ1eT.

Good point. You convinced me.

[orbeckst]

I like @richardjgowers's

• base.ReaderBase
• base.AnalysisBase
• base.XxxBase

Slightly preferred over base.BaseXxx just because I like to read more distinguishing information up-front and I find it aesthetically more pleasing in the context of base.. As @jbarnoud #921 (comment) I am also convinced by the argument that it is still necessary to keep Base in the name for when importing from the base module.

[kain88-de]

OK so we keep @richardjgowers proposal. That leaves the second part of the questions. What is a list of all abstract base classes in MDAnalysis. I would also like to use the list to track progress with #919.

[orbeckst]

Base classes:

coordinates

• coordinates.base.Timestep
• coordinates.base.IObase
• coordinates.base.ProtoReader
• coordinates.base.Reader
• coordinates.base.SingleFrameReader
• coordinates.base.Writer

topology

Not sure if the following still exists in #363:

• topology.base.TopologyReader

selections

• selections.base.SelectionWriter

analysis

• analysis.base.AnalysisBase

[orbeckst]

(Did I forget any?)

[richardjgowers]

So Timestep is fully functional as-is, so isn't really a base like the others (which need to get methods added before use).

@orbeckst TopologyReader still exists and is much the same!

[orbeckst]

@kain88-de you opened this discussion – is there a consensus/conclusion, what are we going to do?