Opened 15 months ago

Closed 13 months ago

#31654 closed enhancement (fixed)

copy_from for scalar fields

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.4
Component: manifolds Keywords: scalarfields
Cc: egourgoulhon, tscrim Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw, Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: e15fbf3 (Commits, GitHub, GitLab) Commit: e15fbf3743e34666aff1b97dfc14d407e269688c
Dependencies: Stopgaps:

Status badges

Description

In this ticket we equip scalar fields with a method copy_from as we did for tensor fields in #28628.

Change History (25)

comment:1 Changed 15 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/copy_from_scalarfields

comment:2 Changed 15 months ago by gh-mjungmath

  • Commit set to 4d6993e51c7b5f6eb4c92a66d9e68a0652a27425
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

New commits:

​4d6993eTrac #31654: add copy_from to scalarfields

comment:3 Changed 15 months ago by git

  • Commit changed from 4d6993e51c7b5f6eb4c92a66d9e68a0652a27425 to 075f512c8c10f66d04a7d1d5110af20984bc047b

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

​075f512Trac #31654: copy of restrictions unnecessary

comment:4 Changed 15 months ago by gh-mjungmath

Copying the restrictions seems unnecessary. All information should be contained in the expressions.

comment:5 Changed 15 months ago by git

  • Commit changed from 075f512c8c10f66d04a7d1d5110af20984bc047b to 3a2f28572f847e20378e715b2a155f3c00cae0f5

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

​3a2f285Trac #31654: minor typo

comment:6 Changed 15 months ago by gh-DaveWitteMorris

  • Branch changed from u/gh-mjungmath/copy_from_scalarfields to public/31654

comment:7 Changed 15 months ago by gh-DaveWitteMorris

  • Branch changed from public/31654 to public/ticket/21991
  • Commit changed from 3a2f28572f847e20378e715b2a155f3c00cae0f5 to 13b8f3fa915439e5edbc2df077c22fc679b6593e

Sorry!!! I pushed my branch to the wrong ticket (typo).

New commits:

​13b8f3f#21991 regression test for issue fixed in #24371

comment:8 Changed 15 months ago by gh-DaveWitteMorris

  • Branch changed from public/ticket/21991 to u/gh-mjungmath/copy_from_scalarfields
  • Commit changed from 13b8f3fa915439e5edbc2df077c22fc679b6593e to 3a2f28572f847e20378e715b2a155f3c00cae0f5

New commits:

​4d6993eTrac #31654: add copy_from to scalarfields
​075f512Trac #31654: copy of restrictions unnecessary
​3a2f285Trac #31654: minor typo

comment:9 Changed 15 months ago by gh-mjungmath

No worries. I am just happy so see that I am not the only one doing these kind of mistkes. :D

Version 0, edited 15 months ago by gh-mjungmath (next)

comment:10 Changed 15 months ago by gh-DaveWitteMorris

Thanks very much for your understanding. Please be sure your ticket is set back to the correct branch. I tried to fix it, but I don't really understand trac, so I'm confused and I think may have made things worse: I set the branch to u/gh-mjungmath/copy_from_scalarfields but it looks like maybe it should be public/ticket/21991. (PS my branch was intended for #31645, and is now there.)

comment:11 Changed 15 months ago by gh-mjungmath

Branch looks fine.

comment:12 Changed 15 months ago by gh-mjungmath

Patchbot is green.

comment:13 Changed 15 months ago by tscrim

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

LGTM.

comment:14 Changed 15 months ago by gh-mjungmath

Thanks for the review!

comment:15 Changed 15 months ago by git

  • Commit changed from 3a2f28572f847e20378e715b2a155f3c00cae0f5 to 3ea7920db5451a1d28d3f604a275c36602d2c1b3
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

​3ea7920Trac #31654: add copy_from to scalarfields

comment:16 Changed 15 months ago by gh-mjungmath

Sorry, I was a little sloppy here and simply copied the code. This name thingy is actually not needed.

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

comment:17 Changed 15 months ago by gh-mjungmath

Thus, needs a new review (waiting for patchbot).

comment:18 Changed 15 months ago by tscrim

green bot => positive review

comment:19 Changed 15 months ago by slelievre

  • Status changed from needs_review to positive_review

comment:20 Changed 15 months ago by slelievre

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Samuel Lelièvre
  • Status changed from positive_review to needs_work

In both tensorfield.py and scalarfield.py, maybe change 

-        Make ``self`` to a copy of ``other``.
+        Make ``self`` a copy of ``other``.

Maybe rephrase this: 

-        - ``other`` -- other scalar field in the very same module from which
-          ``self`` should be a copy of
+        - ``other`` -- other scalar field, in the same module as ``self``

Maybe remove the + sign here? 

             raise TypeError("the original must be an element "
-                            + "of {}".format(self.parent()))
+                            "of {}".format(self.parent()))

Or feel free to ignore these suggestions and move back to positive review.

comment:21 Changed 15 months ago by git

  • Commit changed from 3ea7920db5451a1d28d3f604a275c36602d2c1b3 to e15fbf3743e34666aff1b97dfc14d407e269688c

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

​e15fbf3Trac #31654: docstring improvements

comment:22 Changed 15 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

Done. Thank you. :)

comment:23 Changed 15 months ago by slelievre

  • Status changed from needs_review to positive_review

comment:24 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

comment:25 Changed 13 months ago by vbraun

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