#30799 closed enhancement (fixed)

Add Folder for Manifold Examples

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.3
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8d2f44d (Commits, GitHub, GitLab) Commit: 8d2f44d8c90881202afbca85e3f7e694ac864b27
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-mjungmath)

Belongs to #30189.

I propose to add a new folder for manifold examples:

src/sage/manifolds/differentiable/examples

Furthermore, I suggest we move euclidean.py and real_line.py to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold example can be accessed via the catalog, and the corresponding files are stored in the examples folder.

For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in manifolds, too. But I don't see the need yet.

Change History (57)

comment:1 Changed 12 months ago by gh-mjungmath

  • Description modified (diff)

There are now two way that I see (not related to this ticket directly):

1) The methods in catalog.py operate as a factory method for each model.

2) Each model manages its own factory in its corresponding file (as in EuclideanSpace right now).

Namely, some dimensions have peculiarities that we may want to consider (e.g. S3 is parallelizable).

Last edited 12 months ago by slelievre (previous) (diff)

comment:2 Changed 12 months ago by gh-mjungmath

  • Description modified (diff)

comment:3 Changed 12 months ago by gh-mjungmath

  • Description modified (diff)

comment:4 Changed 12 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/add_folder_for_manifold_models

comment:5 Changed 12 months ago by gh-mjungmath

  • Commit set to 2cbfd307a2d6108f656c9842e0baa8dadb7964f7

Here's my proposal in concrete form. Feedback is welcome.


New commits:

2cbfd30Trac #30799: collect manifold models in 'model' module

comment:6 Changed 12 months ago by git

  • Commit changed from 2cbfd307a2d6108f656c9842e0baa8dadb7964f7 to 96d4382fd90d16061b87780c9e4d8147b2e2f5d0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

96d4382Trac #30799: collect manifold models in 'models' module

comment:7 Changed 12 months ago by gh-mjungmath

I struggle a bit with the word "model". Other ideas are welcome.

comment:8 Changed 12 months ago by tscrim

I don't care for the name "model" as it means something slightly different to me than what you intend. I would say "examples" is a better name.

comment:9 Changed 12 months ago by gh-mjungmath

  • Summary changed from Add Folder for Manifold Models to Add Folder for Manifold Examples

What about calling the folder simply catalog? Or would that mess up the structure with the catalog.py in the manifold's main folder?

comment:10 follow-up: Changed 12 months ago by egourgoulhon

Thanks for this ticket. A folder dedicated to standard manifolds sounds like a good idea. Regarding the name, I would lean to catalog as suggested in comment:9, with an import statement of the form

from sage.manifolds.catalog import Torus   

instead of

from sage.manifolds.differentiable.catalog import Torus   

which looks unnecessarily longer and probably more difficult to find via introspection.

Apparently, there is no catalog folder in all src/sage, only catalog.py files. I don't know if there is any reason for this...

comment:11 Changed 12 months ago by git

  • Commit changed from 96d4382fd90d16061b87780c9e4d8147b2e2f5d0 to 600c646105d5a2e5505a4986ec790e8a4904793c

Branch pushed to git repo; I updated commit sha1. New commits:

600c646Trac #30799: models -> catalog

comment:12 Changed 12 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Status changed from new to needs_review

Doctest works out for manifolds folder. I hope I catched all docstrings refering to EuclideanSpace and real_line.

comment:13 Changed 12 months ago by gh-mjungmath

  • Description modified (diff)

comment:14 Changed 12 months ago by git

  • Commit changed from 600c646105d5a2e5505a4986ec790e8a4904793c to 56148ba303fbfe41c79031d9039d1b69765b4cca

Branch pushed to git repo; I updated commit sha1. New commits:

56148baTrac #30799: add new location to rst file

comment:15 Changed 12 months ago by git

  • Commit changed from 56148ba303fbfe41c79031d9039d1b69765b4cca to 2d49e3e89e9a419827a3030304b4ec827544b1ed

Branch pushed to git repo; I updated commit sha1. New commits:

2d49e3eTrac #30799: imports and docstrings adapted

comment:16 Changed 12 months ago by tscrim

Usually there is just a catalog.py because things are not sufficiently complex to warrant a separate subfolder and there can be simple(-ish) functions that create objects that benefit from being together in one file.

comment:17 Changed 12 months ago by gh-mjungmath

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:18 Changed 12 months ago by git

  • Commit changed from 2d49e3e89e9a419827a3030304b4ec827544b1ed to 7ad486d2a24f86bd91349b0ebca4d71b63f5c1be

Branch pushed to git repo; I updated commit sha1. New commits:

7ad486dTrac #30799: doc adapted

comment:19 Changed 12 months ago by git

  • Commit changed from 7ad486d2a24f86bd91349b0ebca4d71b63f5c1be to 35afb826ee2835dab3b565f4d35fda678509d0cf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

35afb82Trac #30799: doc and imports adapted

comment:20 Changed 12 months ago by gh-mjungmath

Patchbot is green now. :)

comment:21 in reply to: ↑ 10 ; follow-up: Changed 12 months ago by gh-mjungmath

Replying to egourgoulhon:

Thanks for this ticket. A folder dedicated to standard manifolds sounds like a good idea. Regarding the name, I would lean to catalog as suggested in comment:9, with an import statement of the form

from sage.manifolds.catalog import Torus   

instead of

from sage.manifolds.differentiable.catalog import Torus   

which looks unnecessarily longer and probably more difficult to find via introspection.

Apparently, there is no catalog folder in all src/sage, only catalog.py files. I don't know if there is any reason for this...

Btw, in addition to that, the examples can currently be invoked by

manifolds.Torus()

This comes from the fact that all.py imports

import sage.manifolds.catalog as manifolds

I think this is very nice, too.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:22 follow-up: Changed 12 months ago by gh-tobiasdiez

I would suggest to import the different models in the catalog/__init__.py file, so that from a user-perspective there is no real difference in having a big catalog.py file or a catalog module. This would simplify the import

from sage.manifolds.differentiable.catalog.real_line import RealLine

to

from sage.manifolds.differentiable.catalog import RealLine

comment:23 in reply to: ↑ 22 Changed 12 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

I would suggest to import the different models in the catalog/__init__.py file, so that from a user-perspective there is no real difference in having a big catalog.py file or a catalog module. This would simplify the import

from sage.manifolds.differentiable.catalog.real_line import RealLine

to

from sage.manifolds.differentiable.catalog import RealLine

The import pattern is currently

S2 = manifolds.Sphere(2)

and

from sage.manifolds.catalog import Sphere

S2 = Sphere(2)

I think this is what we should promote. As Eric pointed out in comment:10,

from sage.manifolds.differentiable.catalog...

is something that should not be promoted, and I agree with him.

However, even if the import pattern is the same, the suggested folder structure is nice because it indicates what examples are differentiable manifolds and what not.

comment:24 follow-up: Changed 12 months ago by gh-tobiasdiez

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

comment:25 in reply to: ↑ 24 ; follow-up: Changed 12 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

Then I see the problem that the imports rely on the catalog import pattern too much. If we decide to change it at some point, there is too much dependency involved. I personally don't care when the internal import statements are that long.

I think it is always good to import the modules internally from where they are. Besides, I can imagine that the work around could cause a slight slowdown, because the catalog module contains further code which is not necessary for the EuclideanSpace.

Eric, Travis? What do you say?

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:26 Changed 12 months ago by gh-mjungmath

The more I think about it, the more I tend to an examples folder instead. This is at least consistent with other modules (see e.g. homology).

comment:27 in reply to: ↑ 21 Changed 12 months ago by egourgoulhon

Replying to gh-mjungmath:

Btw, in addition to that, the examples can currently be invoked by

manifolds.Torus()

This comes from the fact that all.py imports

import sage.manifolds.catalog as manifolds

I think this is very nice, too.

Ah yes indeed, that's the standard way to use the catalog. Much better than the import statement I mentioned in comment:10. So let us forget about the latter.

comment:28 in reply to: ↑ 25 Changed 12 months ago by egourgoulhon

Replying to gh-mjungmath:

Replying to gh-tobiasdiez:

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

Then I see the problem that the imports rely on the catalog import pattern too much. If we decide to change it at some point, there is too much dependency involved. I personally don't care when the internal import statements are that long.

+1

I think it is always good to import the modules internally from where they are.

Indeed.

comment:29 Changed 12 months ago by git

  • Commit changed from 35afb826ee2835dab3b565f4d35fda678509d0cf to 8d2f44d8c90881202afbca85e3f7e694ac864b27

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8d2f44dTrac #30799: collect manifolds examples in 'examples' + lazy_import for catalog

comment:30 Changed 12 months ago by gh-mjungmath

  • Description modified (diff)

Changed catalog to examples and some further details. Wait for patchbot.

comment:31 follow-up: Changed 12 months ago by egourgoulhon

I am not sure examples is a better name than catalog in the present case. Indeed, this folder shall contain most (all?) "standard" manifolds in low dimension: Euclidian spaces, spheres, tori, projective spaces, etc. It looks more than a mere collection of examples to me.

comment:32 in reply to: ↑ 31 Changed 12 months ago by gh-mjungmath

Replying to egourgoulhon:

I am not sure examples is a better name than catalog in the present case. Indeed, this folder shall contain most (all?) "standard" manifolds in low dimension: Euclidian spaces, spheres, tori, projective spaces, etc. It looks more than a mere collection of examples to me.

The homology module for instance calls their catalog members "examples", too. Moreover, I would prefer examples because it doesn't pollute the namespace too much with the preexisting catalog.py.

comment:33 follow-up: Changed 12 months ago by tscrim

I would also go for examples rather than catalog because we can add functions to the catalog file that are not sufficiently big/detailed to require their own file.

Also, I strongly support directly important from the files at hand. This makes it a little easier to see where cyclic imports are coming from and to help prevent that.

comment:34 follow-up: Changed 12 months ago by gh-tobiasdiez

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module. You can then still easily add functions to catalog/__init__py.

comment:35 in reply to: ↑ 33 Changed 12 months ago by gh-mjungmath

Stalemate! :D

Replying to gh-tobiasdiez:

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module.

Not precisely. As Travis pointed out in comment:33, the catalog.py still contains entire code of "small" examples. Besides, I think it is best if the catalog.py also manages the factories to construct the examples at hand. Let me give two arguments for that:

  1. For example, inheriting from a Manifold class makes that class a UniqueRepresentation. But that is not the behavior we want for examples constructed from catalog.py (compare EuclideanSpace or the Manifold class and the Manifold factory method imported to the global namespace).
  1. The factory method can delegate special dimensions, exotic counterparts or different realizations to the corresponding implementation.

You can then still easily add functions to catalog/__init__py.

I would advice against it. Nobody expects code in __init__.py.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:36 in reply to: ↑ 34 Changed 12 months ago by tscrim

Replying to gh-tobiasdiez:

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module. You can then still easily add functions to catalog/__init__py.

That doesn't quite make sense, catalog.py is a module (in Python-speak). If you mean folder, then no, that is not the long-term goal.

However, there is not any specific reason to not have code in __init__.py.

comment:37 follow-up: Changed 12 months ago by gh-tobiasdiez

Ok, what is then the long-term goal?

  • The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?
  • Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

The factory method can delegate special dimensions, exotic counterparts or different realizations to the corresponding implementation.

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 12 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?

You got a point. The most simple examples, Euclidean space and open intervals, are already complex enough to occupy whole files. On the other hand, some examples might stay primitive and are too short to occupy an own file.

Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

This was exactly my worry and is the reason why I proposed examples as folder name. If we do as you propose, we additionally have to make sure that the documentation about the catalog remains at least somewhere; __init__.py is no option as far as I can see.

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

This sounds like a reasonable approach, too. It is also consistent with the current implementation of EuclideanSpace.

On the other hand, the only thing I'm unsure about is whether this approach is flexible enough for adding new examples/alternatives step-by-step, also by developers who might not be not familiar with our discussion here. As for the factory method this is easy and comprehensible: just add another if-clause and/or option.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:39 in reply to: ↑ 38 Changed 12 months ago by egourgoulhon

Replying to gh-mjungmath:

Replying to gh-tobiasdiez:

The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?

You got a point. The most simple examples, Euclidean space and open intervals, are already complex enough to occupy whole files. On the other hand, some examples might stay primitive and are too short to occupy an own file.

Indeed.

Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

This was exactly my worry and is the reason why I proposed examples as folder name. If we do as you propose, we additionally have to make sure that the documentation about the catalog remains at least somewhere; __init__.py is no option as far as I can see.

OK, to be consistent with the rest of Sage code, it's probably wise to keep the catalog.py file and rename the folder(s) by something else than catalog. I was somewhat reluctant about examples, but at the moment I don't have any better name to suggest (besides models as already suggested by Michael, realizations came to my mind but I am not sure it's pertinent...).

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

This sounds like a reasonable approach, too. It is also consistent with the current implementation of EuclideanSpace.

Yes. Most probably, we need a Sphere2D class, at least for the standard naming of spherical coordinates as (theta, phi). We shall also need special subclasses for parallelizable spheres (S1, S3 and S7).

On the other hand, the only thing I'm unsure about is whether this approach is flexible enough for adding new examples/alternatives step-by-step, also by developers who might not be not familiar with our discussion here. As for the factory method this is easy and comprehensible: just add another if-clause and/or option.

Yes factory approach might be better in this respect. It avoids to invoke __classcall_private__, as currently done in EuclideanSpace.

comment:40 Changed 12 months ago by gh-mjungmath

Allow me to summarize the accumulated discussion. Unfortunately, we can't start polls here (a feature I completely miss).

  • For consistency, to keep the documentation, and to obtain a more comprehensible factory pattern, it is better to keep the catalog.py.
  • In conclusion, we have to choose another name for the folders. Possible suggestions:
    1. models
    2. examples
    3. realizations
  • Which pattern?
    1. Special dimensions of the same implementation could be treated via facade pattern as it has been done for EuclideanSpace, whereas more deviating realizations could be treated via factory method pattern in catalog.py.
    2. Everything is regulated via facade.
    3. Everything is regulated via factory method in catalog.py.

With respect to the last point, I suggest that every cataloged manifold should have it's own method in catalog.py for documentation and unification reasons. This also avoids duplicated or circular imports.


Please let me know if I caught something wrong.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:41 follow-up: Changed 12 months ago by gh-tobiasdiez

Ok, I'm convinced: +1 for examples. I would also tend to put every example in its own file. It's more consistent and I don't see any disadvantages of having "small" classes.

For the pattern, it probably depends on the concrete example and how much the implementations differ. In general, the factory pattern is good when you have completely independent implementations of some interface (e.g. getting infos from a file vs from an web api) whereas the façade pattern excels at hiding the implementation details. You can also combine them easily. For example, for the sphere one could imagine the following:

  • A general class Sphere that acts as a façade for Sphere2Dim and SphereHigherDim (e.g. because you might have more effective ways to calculate in 2d).
  • Classes for d=1,3,7 deriving from Sphere and adding the parallelizable aspect.
  • Factory method in catalog.py that based on the dimension invokes the constructor of Sphere or one of its subclasses for d=1,3,7

comment:42 in reply to: ↑ 41 Changed 12 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

I would also tend to put every example in its own file. It's more consistent and I don't see any disadvantages of having "small" classes.

I disagree here. Dedicated methods to each example are a good thing anyway imho, see comment:40. So why not leave the simple constructions there?

For the pattern, it probably depends on the concrete example and how much the implementations differ. In general, the factory pattern is good when you have completely independent implementations of some interface (e.g. getting infos from a file vs from an web api) whereas the façade pattern excels at hiding the implementation details. You can also combine them easily. For example, for the sphere one could imagine the following:

  • A general class Sphere that acts as a façade for Sphere2Dim and SphereHigherDim (e.g. because you might have more effective ways to calculate in 2d).
  • Classes for d=1,3,7 deriving from Sphere and adding the parallelizable aspect.
  • Factory method in catalog.py that based on the dimension invokes the constructor of Sphere or one of its subclasses for d=1,3,7

Totally! I think this is how we should do it. The parallelizable cases have the same charts, but in contrast to their brothers a global frame which can be constructed in the corresponding classes. So, I'd prefer the façade pattern here as well. But this is something we should discuss in more detail in #30804.

comment:43 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:44 Changed 12 months ago by git

  • Commit changed from 8d2f44d8c90881202afbca85e3f7e694ac864b27 to 167fa25026ce6528c35eac8cca8ef79d6b01f841

Branch pushed to git repo; I updated commit sha1. New commits:

6581bc9Merge branch 'develop' into t/30799/add_folder_for_manifold_models
167fa25Trac #30799: unique_tag for examples

comment:45 follow-ups: Changed 12 months ago by gh-mjungmath

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:46 in reply to: ↑ 45 Changed 12 months ago by gh-mjungmath

Replying to gh-mjungmath:

However, with this commit, there is a doctest error in line 215 of real_line.py...

See #30830 for details.

comment:47 in reply to: ↑ 45 ; follow-up: Changed 12 months ago by gh-mjungmath

Replying to gh-mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too. And for more consistency, it is good to manage this behavior via factory methods.

Please object if you disagree.

comment:48 in reply to: ↑ 47 ; follow-ups: Changed 12 months ago by egourgoulhon

Replying to gh-mjungmath:

Replying to gh-mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too.

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

Anyway, the unique tag issue is beyond the scope of this ticket, I believe. At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

comment:49 in reply to: ↑ 48 ; follow-up: Changed 12 months ago by gh-mjungmath

Replying to egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

Anyway, the unique tag issue is beyond the scope of this ticket, I believe. At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

You're right. However, it is somehow entwined. It wouldn't make sense to create a factory method somewhere else than in catalog.py.

comment:50 in reply to: ↑ 49 ; follow-up: Changed 12 months ago by egourgoulhon

Replying to gh-mjungmath:

Replying to egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

RealLine() or OpenInterval(0,1) fully defines the mathematical object, Manifold(2, 'M') does not.

comment:51 in reply to: ↑ 50 Changed 12 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-mjungmath:

Replying to egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

RealLine() or OpenInterval(0,1) fully defines the mathematical object, Manifold(2, 'M') does not.

Mh, I see. And EuclideanSpace(2) could still yield two different but isomorphic mathematical entities.

But what about OpenInterval(0,1) and OpenInterval(0,1, name='I'). Or OpenInterval(-oo,oo) and RealLine(). Same mathematical object, but different instances.

I think, this should be catched via __classcall_private__ then. However, that's definitely beyond the scope of this ticket.


So, what's your proposal with the import/method/catalog behavior then? Keep the version of comment:29?

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:52 in reply to: ↑ 48 Changed 12 months ago by tscrim

Replying to egourgoulhon:

Replying to gh-mjungmath:

Replying to gh-mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too.

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

Anyway, the unique tag issue is beyond the scope of this ticket, I believe. At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

I still haven't forgotten that I promised you I would do that. It is on my to-do list. Sorry for taking so long.

comment:53 Changed 12 months ago by git

  • Commit changed from 167fa25026ce6528c35eac8cca8ef79d6b01f841 to 8d2f44d8c90881202afbca85e3f7e694ac864b27

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:54 Changed 12 months ago by gh-mjungmath

Like Eric pointed out correctly, some things discussed here go beyond the scope of this ticket. I reverted the branch to an older version, where I used lazy imports into catalog.py.

Lazy import should prevent circular and unneccessary imports.

Patchbot is already green.

comment:55 Changed 12 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

We can do further changes in other tickets. I think it is good to take some smaller steps in each ticket.

comment:56 Changed 12 months ago by gh-mjungmath

I agree.

Thanks.

comment:57 Changed 12 months ago by vbraun

  • Branch changed from u/gh-mjungmath/add_folder_for_manifold_models to 8d2f44d8c90881202afbca85e3f7e694ac864b27
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.