Opened 4 months ago

Closed 3 months ago

#30094 closed defect (fixed)

Basis-dependent isomorphism from FiniteRankFreeModule to an object in the category ModulesWithBasis

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: categories Keywords:
Cc: egourgoulhon, tscrim, gh-mjungmath Merged in:
Authors: Matthias Koeppe, Michael Jung Reviewers: Michael Jung, Matthias Koeppe, Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8a800cc (Commits) Commit: 8a800cc0734fe2c0c9e4e201faf5b9435fdab78e
Dependencies: #30180 Stopgaps:

Description (last modified by mkoeppe)

(This ticket has been narrowed to the topic mainly discussed in the comments. The topic originally in title and ticket description has been moved to #30174)

There are multiple incompatible protocols for dealing with bases of a free module in Sage (#19346).

One such protocol is defined by the category ModulesWithBasis. CombinatorialFreeModule is one of the parent classes in this category.

Another such protocol is defined by the parent class FiniteRankFreeModule, which is in the category Modules. A FiniteRankFreeModule can be equipped with a finite list of bases (each represented by an instance of FreeModuleBasis), none of which are considered distinguished. FiniteRankFreeModules are unique parents; the operation of equipping a FiniteRankFreeModule does not change the identity of the parent.

We define a method FiniteRankFreeModule. isomorphism_with_fixed_basis that establishes, given a basis, an isomorphism with a CombinatorialFreeModule (and thus a parent in the category ModulesWithBasis.

Change History (68)

comment:1 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 4 months ago by mkoeppe

Also it seems various maps are missing.

sage: Q3.has_coerce_map_from(QQ^3)
False
sage: Q3((QQ^3).an_element())
ValueError: the None has not been defined on the 3-dimensional vector space over the Rational Field

comment:3 Changed 3 months ago by mkoeppe

  • Cc gh-mjungmath added

comment:4 follow-ups: Changed 3 months ago by tscrim

These are incompatible objects since QQ^3 has a distinguished basis (the standard Euclidean basis) whereas Q3 does not. So there should not be a map between them.

My 2 cents for the error reported in the ticket is to first check that the object is not in the category. The other option is to lift the vector_space method up to the category of modules and then implement a change_ring method for Q3.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 months ago by mkoeppe

Replying to tscrim:

These are incompatible objects since QQ^3 has a distinguished basis (the standard Euclidean basis) whereas Q3 does not. So there should not be a map between them.

Shouldn't there be a map in one direction that forgets the distinguished basis?

comment:6 in reply to: ↑ 5 Changed 3 months ago by tscrim

Replying to mkoeppe:

Replying to tscrim:

These are incompatible objects since QQ^3 has a distinguished basis (the standard Euclidean basis) whereas Q3 does not. So there should not be a map between them.

Shouldn't there be a map in one direction that forgets the distinguished basis?

No, because where would you send the point (1,3,1) to? How would you know how it is expressed in one of the bases of Q3?

comment:7 Changed 3 months ago by mkoeppe

Ok, I feel we had this discussion before

comment:8 follow-up: Changed 3 months ago by mkoeppe

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 months ago by gh-mjungmath

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

No, I don't think it should. The identity map (like any other automorphism) is a basis independent object.

comment:10 in reply to: ↑ 9 ; follow-ups: Changed 3 months ago by tscrim

Replying to gh-mjungmath:

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

No, I don't think it should. The identity map (like any other automorphism) is a basis independent object.

I might even go further to say that it would not be the identity map, but an isomorphism of free modules. We have to be careful about distinguishing things up to equality and up to isomorphism. For example, if I have V = span_R{x,y,z}, this is not equal to W = R^3. So there is no identity map from V -> W because I am not looking at End(V), but Mor(V, W). Yes, they are isomorphic, but there is no canonical isomorphism until I choose a distinguished basis for V and W.

comment:11 in reply to: ↑ 10 Changed 3 months ago by mkoeppe

Replying to tscrim:

Replying to gh-mjungmath:

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

No, I don't think it should. The identity map (like any other automorphism) is a basis independent object.

I might even go further to say that it would not be the identity map, but an isomorphism of free modules.

Of course, that's what I meant.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 3 months ago by mkoeppe

Replying to tscrim:

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

We have to be careful about distinguishing things up to equality and up to isomorphism. For example, if I have V = span_R{x,y,z}, this is not equal to W = R^3. ... there is no canonical isomorphism until I choose a distinguished basis for V and W.

In my question above, this would be a method of FreeModuleBasis - so there is a particular basis. And RR^3 also has a distinguished basis.

Last edited 3 months ago by mkoeppe (previous) (diff)

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 months ago by tscrim

Replying to mkoeppe:

Replying to tscrim:

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

We have to be careful about distinguishing things up to equality and up to isomorphism. For example, if I have V = span_R{x,y,z}, this is not equal to W = R^3. ... there is no canonical isomorphism until I choose a distinguished basis for V and W.

In my question above, this would be a method of FreeModuleBasis - so there is a particular basis. And RR^3 also has a distinguished basis.

You were asking for a map from self.free_module(), which is the free module without a distinguished basis. If you want to go from a FreeModuleBasis, then that might be a good method as there is a canonical isomorphism of the homspace with the endspace and an identity map. Although I am not sure how much the basis is intended as a basis as a set or the span of the basis elements. I suspect the former, so such a method would not make sense. I would have to see what Eric and Michal say.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 months ago by egourgoulhon

Replying to tscrim:

Replying to mkoeppe:

Replying to tscrim:

Replying to mkoeppe:

Should FreeModuleBasis get a method that creates the identity map from self.free_module() to a free module with that basis?

We have to be careful about distinguishing things up to equality and up to isomorphism. For example, if I have V = span_R{x,y,z}, this is not equal to W = R^3. ... there is no canonical isomorphism until I choose a distinguished basis for V and W.

In my question above, this would be a method of FreeModuleBasis - so there is a particular basis. And RR^3 also has a distinguished basis.

You were asking for a map from self.free_module(), which is the free module without a distinguished basis. If you want to go from a FreeModuleBasis, then that might be a good method as there is a canonical isomorphism of the homspace with the endspace and an identity map. Although I am not sure how much the basis is intended as a basis as a set or the span of the basis elements. I suspect the former, so such a method would not make sense. I would have to see what Eric and Michal say.

I of course agree that one may implement a method in FreeModuleBasis that returns an isomorphism between Q3 and QQ^3, but for which purpose? Such an isomorphism cannot be used for a coercion since a coercion must be unique and cannot depend on a extra parameter such as a basis-dependent isomorphism.

Coming back to the issue mentioned in the ticket title and description, I would say that the problem arises because "vector spaces" as they are implemented in Sage are not really vector spaces in all their generality but only Cartesian powers F^n, where F is the base field.

comment:15 in reply to: ↑ 14 ; follow-ups: Changed 3 months ago by mkoeppe

Replying to egourgoulhon:

Coming back to the issue mentioned in the ticket title and description, I would say that the problem arises because "vector spaces" as they are implemented in Sage are not really vector spaces in all their generality but only Cartesian powers F^n, where F is the base field.

OK, I think we are getting closer.

When you say, '"vector spaces" as they are implemented in Sage', I assume you mean the ones implemented in sage.modules.free_module and constructed by the function VectorSpace, and then I agree with what you wrote.

What I was asking for in comment 8 is, given a basis, a map from a FiniteRankFreeModule (which is in the category of finite dimensional modules) to a parent that is in the category of finite dimensional modules with basis.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 months ago by gh-mjungmath

Replying to tscrim:

I might even go further to say that it would not be the identity map, but an isomorphism of free modules.

Well, yes. It is a special kind of automorphism. However, I wouldn't see it as an isomorphism between any two free modules of the same rank, in case this is what you are referring to.

Replying to tscrim:

Although I am not sure how much the basis is intended as a basis as a set or the span of the basis elements. I suspect the former, so such a method would not make sense. I would have to see what Eric and Michal say.

I am not sure what you mean with "the span of the basis elements". This does not make any sense to me because the span is always the free module again. Anyway, I would see an instance of FreeModuleBasis as a representation of a (finite) subset of the corresponding free module which is linearly independent and generates the module (possibly in the sense of formal linear combinations).

comment:17 in reply to: ↑ 4 Changed 3 months ago by gh-mjungmath

Replying to tscrim:

These are incompatible objects since QQ^3 has a distinguished basis (the standard Euclidean basis) whereas Q3 does not. So there should not be a map between them.

Agreed.

My 2 cents for the error reported in the ticket is to first check that the object is not in the category. The other option is to lift the vector_space method up to the category of modules and then implement a change_ring method for Q3.

Sounds good. Who goes for it?

comment:18 in reply to: ↑ 15 ; follow-up: Changed 3 months ago by egourgoulhon

Replying to mkoeppe:

Replying to egourgoulhon:

Coming back to the issue mentioned in the ticket title and description, I would say that the problem arises because "vector spaces" as they are implemented in Sage are not really vector spaces in all their generality but only Cartesian powers F^n, where F is the base field.

OK, I think we are getting closer.

When you say, '"vector spaces" as they are implemented in Sage', I assume you mean the ones implemented in sage.modules.free_module and constructed by the function VectorSpace,

Yes.

and then I agree with what you wrote.

What I was asking for in comment 8 is, given a basis, a map from a FiniteRankFreeModule (which is in the category of finite dimensional modules) to a parent that is in the category of finite dimensional modules with basis.

Yes, this is the basis-dependent isomorphism I was referring to comment:14, but my concern, and that of Travis if I understand correctly, is that such an isomorphism cannot be used for a coercion, because it is not canonical (by definition, it depends on a choice of basis).

comment:19 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/unable_to_coerce_x___3_dimensional_vector_space_over_the_rational_field__into_category_of_vector_spaces_over_rational_field

comment:20 in reply to: ↑ 18 Changed 3 months ago by mkoeppe

  • Commit set to 91e44f4479bfb30663873faa7de0467e542daebb

Replying to egourgoulhon:

Yes, this is the basis-dependent isomorphism I was referring to comment:14, but my concern, and that of Travis if I understand correctly, is that such an isomorphism cannot be used for a coercion, because it is not canonical (by definition, it depends on a choice of basis).

I've pushed a rough implementation of this to the ticket.

Sure, I don't need this to be a coercion. Just a module morphism.


New commits:

91e44f4sage.tensor.modules.FiniteRankFreeModule.basis_fixing_map: New

comment:21 in reply to: ↑ 16 ; follow-up: Changed 3 months ago by tscrim

Replying to gh-mjungmath:

Replying to tscrim:

Although I am not sure how much the basis is intended as a basis as a set or the span of the basis elements. I suspect the former, so such a method would not make sense. I would have to see what Eric and Michal say.

I am not sure what you mean with "the span of the basis elements". This does not make any sense to me because the span is always the free module again. Anyway, I would see an instance of FreeModuleBasis as a representation of a (finite) subset of the corresponding free module which is linearly independent and generates the module (possibly in the sense of formal linear combinations).

The question is what does this class represent:

  1. The basis itself, which is a finite set.
  2. The span of the basis elements, which is a vector space where we can represent the points in terms of the chosen basis elements.

These are very different objects. One is finite, one is infinite (well, |R|n, where R is the base ring). Since you want to think of it as 1, then you are saying there should not be a morphism. However, most of the points of the free module should not be elements of this parent. The actual implementation is really treating it like 2, and its elements are the points of the free module represented using the chosen basis.


I like the proposed method overall. I have some suggestions:

1 - Change the name to something like morphism_with_fixed_basis as right now it sounds like it is constructing a map that happens to fix a basis. 2 - Allow the codomain of the map to be passed to the function. Then you can have the map be defined as

        if codomain.dimension() != self.dimension():
            raise ValueError
        B = list(codomain.basis())
        def function(x):
            return codomain.sum(x[basis, i] * B[i] for i in self.irange())

This should be implementation independent (if it is not, then it is a bug IMO).

comment:22 in reply to: ↑ 21 Changed 3 months ago by mkoeppe

Replying to tscrim:

Replying to gh-mjungmath:

Replying to tscrim:

Although I am not sure how much the basis is intended as a basis as a set or the span of the basis elements. I suspect the former, so such a method would not make sense. I would have to see what Eric and Michal say.

I am not sure what you mean with "the span of the basis elements". This does not make any sense to me because the span is always the free module again. Anyway, I would see an instance of FreeModuleBasis as a representation of a (finite) subset of the corresponding free module which is linearly independent and generates the module (possibly in the sense of formal linear combinations).

The question is what does this class represent:

  1. The basis itself, which is a finite set.
  2. The span of the basis elements, which is a vector space where we can represent the points in terms of the chosen basis elements.

I think it's clear that 1 is intended:

sage: V = FiniteRankFreeModule(QQ, 3)
sage: e = V.basis("e")
sage: e[0] in e
True
sage: e[1] in e
True
sage: e[0] + e[1] in e
False

comment:23 follow-up: Changed 3 months ago by git

  • Commit changed from 91e44f4479bfb30663873faa7de0467e542daebb to 73667410fb02014b789a49ef58e9846ecc729419

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

7366741Rename, generalize

comment:24 Changed 3 months ago by tscrim

Ah, right. Yes, it is clear that it is 1. I got myself confused.

Last edited 3 months ago by tscrim (previous) (diff)

comment:25 in reply to: ↑ 23 ; follow-ups: Changed 3 months ago by gh-mjungmath

Replying to git:

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

7366741Rename, generalize

I must agree Eric in comment:14, I don't see the use of such a method isomorphism_with_fixed_basis, not even in FiniteRankFreeModule.

I see no problem in implementing the coercion on the level of modules right away. There is no need for such method either in this case. The vector space QQ^3 forgets about the basis we have chosen, anyway (i.e. there is no going back). The important thing is that we are able to choose a basis. But this is ensured due to the definition of a free module.

I have to admit, I am not sure what's the problem is we're still discussing; apart from the error reported in the description. Is it still all about comment:2? As reported in this ticket, we want to coerce the free modules themselves and not their elements.

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

comment:26 in reply to: ↑ 25 Changed 3 months ago by mkoeppe

Replying to gh-mjungmath:

I have to admit, I am not sure what's the problem is we're still discussing

Thanks. I will split the ticket into two and clarify the descriptions.

comment:27 Changed 3 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from unable to coerce x (=3-dimensional vector space over the Rational Field) into Category of vector spaces over Rational Field to Basis-dependent isomorphism from FiniteRankFreeModule to an object in the category ModulesWithBasis

comment:28 in reply to: ↑ 25 Changed 3 months ago by mkoeppe

The topic originally on the ticket description is now in #30174. I hope this helps.

comment:29 Changed 3 months ago by gh-mjungmath

Looking at the code, I begin to like the idea, too. :)

It's still a draft, however I'd like to propose an improvement regarding the docstring:

         Construct the canonical isomorphism from the free module ``self``
-        to a free module in which ``basis`` of ``self`` is the distinguished basis.
+        to a free module in which ``basis`` of ``self`` is mapped to the
+        distinguished basis of ``codomain``.

I think it would also be good to specify input and output in the docstring. In particular, the input codomain should be described as an element of ModulesWithBasis having the same rank as self and over the same base ring. Furthermore, it should be explained that CombinatorialFreeModule is chosen by default if the input is None.

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

comment:30 Changed 3 months ago by gh-mjungmath

And probably we should raise an error in case both base rings are not equal.

comment:31 Changed 3 months ago by mkoeppe

Thanks for the suggestions, sounds all good. I don't have time to work on it today. Please feel free to commit changes to the branch

comment:32 Changed 3 months ago by gh-mjungmath

  • Branch changed from u/mkoeppe/unable_to_coerce_x___3_dimensional_vector_space_over_the_rational_field__into_category_of_vector_spaces_over_rational_field to public/tensor/basis_dependent_isomorphism_free_modules
  • Commit 73667410fb02014b789a49ef58e9846ecc729419 deleted

comment:33 Changed 3 months ago by git

  • Commit set to 81e0ec0c1902a55269214f81e2cce130253b0de8

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

91e44f4sage.tensor.modules.FiniteRankFreeModule.basis_fixing_map: New
7366741Rename, generalize
81e0ec0Trac #30094: docstring improved + raise error for different base rings

comment:34 Changed 3 months ago by git

  • Commit changed from 81e0ec0c1902a55269214f81e2cce130253b0de8 to bdc6e2c8194911acf4e2a6a021e2f417504b9752

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

bdc6e2cTrac #30094: minor improvements in docstring

comment:35 follow-up: Changed 3 months ago by egourgoulhon

Thanks for the code! I think this should be changed:

-            return codomain.sum(x[basis, i] * codomain_basis[i]
-                                for i in self.irange())
+            return codomain.sum(x[basis, i] * codomain_basis[i - self._sindex]
+                                for i in self.irange())

The doctest

sage: self = V = FiniteRankFreeModule(QQ, 3); V

could be changed to

sage: self = V = FiniteRankFreeModule(QQ, 3, start_index=1); V

to check that everything is OK with index ranges.

comment:36 Changed 3 months ago by git

  • Commit changed from bdc6e2c8194911acf4e2a6a021e2f417504b9752 to acaaa7fbb60c891ba80ecf504430a7c68c99dad9

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

acaaa7fTrac #30094: starting index corrected

comment:37 in reply to: ↑ 35 Changed 3 months ago by gh-mjungmath

Replying to egourgoulhon:

Thanks for the code! I think this should be changed:

-            return codomain.sum(x[basis, i] * codomain_basis[i]
-                                for i in self.irange())
+            return codomain.sum(x[basis, i] * codomain_basis[i - self._sindex]
+                                for i in self.irange())

The doctest

sage: self = V = FiniteRankFreeModule(QQ, 3); V

could be changed to

sage: self = V = FiniteRankFreeModule(QQ, 3, start_index=1); V

to check that everything is OK with index ranges.

Very good objection. I took that into consideration. I think, the ticket is ready for review now. Matthias?

comment:38 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe, Michael Jung
  • Status changed from new to needs_review

comment:39 Changed 3 months ago by mkoeppe

I'm happy with these changes, thanks very much!

comment:40 follow-up: Changed 3 months ago by mkoeppe

Of course, one could ask what the basis of the CombinatorialFreeModule should be indexed by.

sage: V = FiniteRankFreeModule(QQ, 3, start_index=1)
sage: basis = e = V.basis("e")
sage: phi_e.codomain().basis()
Finite family {'e_1': B['e_1'], 'e_2': B['e_2'], 'e_3': B['e_3']}

I think it would be more natural if this family was indexed by V.irange().

comment:41 in reply to: ↑ 40 Changed 3 months ago by gh-mjungmath

Replying to mkoeppe:

Of course, one could ask what the basis of the CombinatorialFreeModule should be indexed by.

sage: V = FiniteRankFreeModule(QQ, 3, start_index=1)
sage: basis = e = V.basis("e")
sage: phi_e.codomain().basis()
Finite family {'e_1': B['e_1'], 'e_2': B['e_2'], 'e_3': B['e_3']}

I think it would be more natural if this family was indexed by V.irange().

I don't quite get what you want. I have quickly overviewd the documentation of CombinatorialFreeModule, and I think that our implementation already carries out the intended behavior.

comment:42 follow-up: Changed 3 months ago by mkoeppe

sage: phi_e.codomain().basis().keys()
{'e_1', 'e_2', 'e_3'}

I think the code should be changed so that

sage: phi_e.codomain().basis().keys()
{1, 2, 3}

comment:43 in reply to: ↑ 42 Changed 3 months ago by gh-mjungmath

Replying to mkoeppe:

sage: phi_e.codomain().basis().keys()
{'e_1', 'e_2', 'e_3'}

I think the code should be changed so that

sage: phi_e.codomain().basis().keys()
{1, 2, 3}

As far as I understand it, the keys correspond to the basis element's name. And I thought that the intention was to name them after the basis we are coming from, wasn't it?

Notice that the names can also differ from the canonical choice, for example a, b, c, without labeled indices. In that case, the current implementation is preferrable anyway.

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

comment:44 Changed 3 months ago by mkoeppe

Are there methods in FreeModuleBasis or FiniteRankFreeModule that allow the user to look up a basis element by name?

comment:45 Changed 3 months ago by gh-mjungmath

I am sorry. I am little off track. You were obiously right:

basis_keys - list, tuple, family, set, etc. defining the indexing set for the basis of this module

What you suggest sounds good then.

comment:46 Changed 3 months ago by git

  • Commit changed from acaaa7fbb60c891ba80ecf504430a7c68c99dad9 to 81fa95dd8abee38ce70b57d35f1aa3c8667c7ffd

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

81fa95dTrac #30094: indexing improved

comment:47 Changed 3 months ago by git

  • Commit changed from 81fa95dd8abee38ce70b57d35f1aa3c8667c7ffd to 5657108b4d0ea40f08e79de42e1a7675dd2f2400

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

5657108Add an example with a provided codomain

comment:48 Changed 3 months ago by mkoeppe

Perhaps the prefix "B" should be changed to basis._symbol

comment:49 follow-up: Changed 3 months ago by git

  • Commit changed from 5657108b4d0ea40f08e79de42e1a7675dd2f2400 to 40f589f9033607bae86eec49b1eeb2d020003880

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

40f589fUse basis._symbol as prefix instead of B

comment:50 in reply to: ↑ 49 Changed 3 months ago by gh-mjungmath

Replying to git:

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

40f589fUse basis._symbol as prefix instead of B

Replying to mkoeppe:

Perhaps the prefix "B" should be changed to basis._symbol

This won't work:

sage: M = FiniteRankFreeModule(QQ, 3)
sage: e = M.basis(['a', 'b', 'c'], symbol_dual=['d', 'e', 'f'])
sage: phi_e = phi_e = M.isomorphism_with_fixed_basis(e)
sage: phi_e.codomain().basis()
<repr(<sage.sets.family.FiniteFamily_with_category at 0x7f03d6b8bb88>) failed: TypeError: can only concatenate tuple (not "str") to tuple>

See comment:43.

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

comment:51 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_review to needs_work

comment:52 Changed 3 months ago by git

  • Commit changed from 40f589f9033607bae86eec49b1eeb2d020003880 to affd0c974f144ca19f2095f3546130f04b188823

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

affd0c9Use basis._symbol as prefix only when it is a string

comment:53 Changed 3 months ago by mkoeppe

  • Status changed from needs_work to needs_review

Thanks for catching this. Here's a better version

comment:54 Changed 3 months ago by gh-mjungmath

I don't think the prefix is even necessary, but I can live with that.

comment:55 Changed 3 months ago by mkoeppe

  • Reviewers set to Michael Jung, Matthias Koeppe, ...

comment:56 Changed 3 months ago by egourgoulhon

  • Reviewers changed from Michael Jung, Matthias Koeppe, ... to Michael Jung, Matthias Koeppe, Eric Gourgoulhon
  • Status changed from needs_review to positive_review

LGTM. Thanks!

comment:57 follow-up: Changed 3 months ago by egourgoulhon

  • Status changed from positive_review to needs_info

Shouldn't the code of this ticket use #30180 (cf. comment 1 in that ticket)? Then #30180 should be added as a dependency.

comment:58 in reply to: ↑ 57 ; follow-up: Changed 3 months ago by gh-mjungmath

Replying to egourgoulhon:

Shouldn't the code of this ticket use #30180 (cf. comment 1 in that ticket)? Then #30180 should be added as a dependency.

I think, the independence should be the other way around, right Matthias?

comment:59 in reply to: ↑ 58 ; follow-up: Changed 3 months ago by egourgoulhon

Replying to gh-mjungmath:

Replying to egourgoulhon:

Shouldn't the code of this ticket use #30180 (cf. comment 1 in that ticket)? Then #30180 should be added as a dependency.

I think, the independence should be the other way around, right Matthias?

No, if the code in this ticket makes use of #30180's code, the dependency shall be set here to #30180.

comment:60 in reply to: ↑ 59 Changed 3 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-mjungmath:

Replying to egourgoulhon:

Shouldn't the code of this ticket use #30180 (cf. comment 1 in that ticket)? Then #30180 should be added as a dependency.

I think, the independence should be the other way around, right Matthias?

No, if the code in this ticket makes use of #30180's code, the dependency shall be set here to #30180.

This code works independently of #30180 and I don't see any snippets from #30180 used here.

comment:61 Changed 3 months ago by mkoeppe

Both of you are right.

comment:62 Changed 3 months ago by mkoeppe

Thanks to the speedy review of #30180, I will use it now to simplify the code on the present ticket.

comment:63 Changed 3 months ago by git

  • Commit changed from affd0c974f144ca19f2095f3546130f04b188823 to 8a800cc0734fe2c0c9e4e201faf5b9435fdab78e

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

0afa01cModules: Add parent method module_morphism
3c8d080Fix docstring markup
e2fc5d5Make sure that we only create a module morphism
68da24cMerge branch 't/30180/category_modules_should_provide_a_parent_method_module_morphism_compatible_with_moduleswithbasis_module_morphism' into t/30094/public/tensor/basis_dependent_isomorphism_free_modules
8a800ccFiniteRankFreeModule.isomorphism_with_fixed_basis: Simplify using self.module_morphism

comment:64 Changed 3 months ago by mkoeppe

  • Dependencies set to #30180
  • Status changed from needs_info to needs_review

comment:65 Changed 3 months ago by tscrim

Morally green patchbot.

comment:66 Changed 3 months ago by egourgoulhon

  • Reviewers changed from Michael Jung, Matthias Koeppe, Eric Gourgoulhon to Michael Jung, Matthias Koeppe, Eric Gourgoulhon, Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks for the update!

comment:67 Changed 3 months ago by mkoeppe

Thanks!

comment:68 Changed 3 months ago by vbraun

  • Branch changed from public/tensor/basis_dependent_isomorphism_free_modules to 8a800cc0734fe2c0c9e4e201faf5b9435fdab78e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.