Opened 11 years ago

Closed 7 years ago

#10672 closed enhancement (fixed)

Categories for FreeModule's

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-6.10
Component: categories Keywords: FreeModule, ModulesWithBasis
Cc: darij Merged in:
Authors: Travis Scrimshaw Reviewers: Darij Grinberg
Report Upstream: N/A Work issues:
Branch: d94f997 (Commits, GitHub, GitLab) Commit: d94f997e5d8082d80cf07067cab8ae13d67fe866
Dependencies: #18066 Stopgaps:

Status badges

Description

Put FreeModule? in the category ModulesWithBasis?(), and implement all the related methods. This should make FreeModule? and CombinatorialFreeModule? compatible and consistent.

There might be some compatibility issues to handle.

Attachments (1)

trac_10672-free_module_category-ts.patch (985 bytes) - added by tscrim 9 years ago.

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by tscrim

comment:1 Changed 9 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Milestone changed from sage-wishlist to sage-5.12
  • Status changed from new to needs_review

I put VectorSpaces in the category ModulesWithBasis because FreeModules is already an alias for ModulesWithBasis. There doesn't seem to be any compatibility issues that I noticed.

comment:2 Changed 9 years ago by nthiery

Thanks for working on this! A big part of the ticket is in "and implement all the related methods". The point is to have all the zillion little utilities of ModulesWithBasis? (term, monomial, sum_of_terms, leading_term, iter, ...) also work for plain FreeModules?.

My guess is that the main incompatibility that needs to be discussed is about iter. Indeed, as far as I remember:

      for x in v:
          ...

iterates over pairs for "Combinatorial" free modules(like a iteritems of dictionaries), whereas it iterates over coefficients for plain free modules. So we have to decide on which interface we want and how to handle backward compatibility.

Cheers

Granted: ModulesWithBasis? should have appropriate abstract methods and tests so that, if an implementation does not provide the required methods, the testsuite barks.

comment:3 Changed 8 years ago by ncohen

  • Status changed from needs_review to needs_work

As the patchbot indicates, this patch does not pass tests. Though it seems good otherwise :-P

Nathann

comment:4 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:6 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:7 Changed 7 years ago by nthiery

See also http://trac.sagemath.org/ticket/18310#comment:13 for a few issues we will need to double check for.

comment:8 Changed 7 years ago by tscrim

  • Branch set to public/categories/last_compatibility-10672
  • Cc darij added
  • Commit set to d94f997e5d8082d80cf07067cab8ae13d67fe866
  • Dependencies set to #18066
  • Milestone changed from sage-6.4 to sage-6.10
  • Status changed from needs_work to needs_review

There is still a minor issue with __iter__, however we can avoid those in code by using monomial_coefficients() which give the consistent behavior. I also think we can leave the issues raised in #18310 to be handled there and once submodule supports coordinate rings...


Last 10 new commits:

4a9fd81Trac 18411: merge Sage 6.10.beta0
525d038Trac 18411: Fix doctest in colored_permutations.py
41af762Trac 18411: pass keywords in __call__
6e27ddeTrac 18411: merge sage-6.10.beta1
b21396cMerge branch '18411' into HEAD
b32e479further changes (mostly doc)
dbb5789Further fixes
e2fb14aremove _test_keytype since submodules of free modules are currently failing it
96004dfDarij...
d94f997Some last compatibility issues with modules with basis (hopefully).

comment:9 Changed 7 years ago by darij

  • Status changed from needs_review to positive_review

comment:10 Changed 7 years ago by darij

LGTM (it is really just the last commit; everything else is from #18066).

The heisenberg.py part of #16820 relies on this patch, so that'll serve as a doctest.

comment:11 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:12 Changed 7 years ago by tscrim

  • Reviewers set to Darij Grinberg
  • Status changed from needs_work to positive_review

comment:13 Changed 7 years ago by vbraun

  • Branch changed from public/categories/last_compatibility-10672 to d94f997e5d8082d80cf07067cab8ae13d67fe866
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.