Quick fix for circular imports

[max-veit]

Fix #294

I can't guarantee that this fixes all our circular import problems, but I think this addresses the one triggered by trying to run

from rascal.models import <something>

I'm not sure how exactly to test this -- it pops up pretty randomly when changing something in the Python code, and depends on import ordering (which is a nightmare), so somehow it has not been caught by the Python binding tests until now. Suggestions welcome.

[max-veit]

And a note for the future: This whole mess was mostly caused by creating "convenience imports" that just rename existing functions in librascal. Let's try to avoid those and try to keep our internal dependency graph as simple as possible to avoid running into such issues again.

[max-veit]

Please note that I had to take the SparsePoints alias-import out of bindings/rascal/models/__init__.py in order to resolve the circular import, so you can no longer just do from rascal.models import SparsePoints. The correct way to import these is now from rascal.models.sparse_points import SparsePoints. The examples and tests have been updated accordingly.

Note that this style of "aliasing" (importing) objects into a higher package level's __init__.py file is generally discouraged for exactly this reason, so eventually we'll want to take the rest of those alias-imports out, especially if they cause problems in the future.

[Luthaf]

The fix looks good as far as fixing circular imports go.

Note that this style of "aliasing" (importing) objects into a higher package level's init.py file is generally discouraged for exactly this reason, so eventually we'll want to take the rest of those alias-imports out, especially if they cause problems in the future.

What makes you think this is discouraged?

I generally prefer to use it as much as possible because it is nicer from the user perspective, which in turns force me to structure my classes and modules as a cycle-free graph (usually a tree). If necessary, I then use late imports (directly inside class methods) to break up module cycles. At the same time, I've mostly worked with relatively small python packages, so I'd like to read more on this subject if you have links!

[Luthaf]

I don't really see why we have this function in librascal ... Is this used anywhere else in the code or is this just a convenience function for librascal users?

[max-veit]

It was used to decide whether to import tqdm in the command-line version or the notebook version, in one of the tutorials (yet to be merged, I think). I know we wanted to make the dependency on tqdm optional, but if it is present, we will probably want to import the appropriate version.

[max-veit]

It was one of the i-PI tutorials actually, see e.g.: af2c7bd

[Luthaf]

Ok, ideally I would move this function to the tutorial code itself, and not inside librascal. But this can happen as part of #307

[max-veit]

That PR is scheduled to be merged soon anyway (I split this fix off from that PR because I thought it was more urgent)

[Luthaf]

Looks good! Feel free to merge once CI is happy!

Also, please squash the commits before or during merge 😃

[max-veit]

What makes you think this is discouraged?

I might be misremembering a blog post or StackOverflow answer I read once, which of course I can't find again. Although I think this one does a pretty good job of explaining the issue and various fixes.

I generally prefer to use it as much as possible because it is nicer from the user perspective, which in turns force me to structure my classes and modules as a cycle-free graph (usually a tree).

Agree, I was hoping this was one step towards making our dependency graph easier to understand.

If necessary, I then use late imports (directly inside class methods) to break up module cycles. At the same time, I've mostly worked with relatively small python packages, so I'd like to read more on this subject if you have links!

I don't like late imports because then you can no longer see the module's dependencies just by looking at the top of the file. Note that there were already several instances of SparsePoints being imported within a function, which might actually make things worse if the import error is only triggered on running a function... Anyway, I took those out, and since SparsePoints is a fairly low-level object anyway I don't think we really need to expose it in models.